Message ID | 20200415141534.31240-9-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | gpio: dwapb: Clean up the driver and a fix | expand |
On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote: > IRQ core provides macros such as IRQ_RETVAL(). > Convert code to use them. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Tested-by: Serge Semin <fancer.lancer@gmail.com> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/gpio/gpio-dwapb.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 8b30ded9322a..4edac592c253 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -258,8 +258,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > irq_hw_number_t bit = irqd_to_hwirq(d); > unsigned long level, polarity, flags; > > - if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING | > - IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) > + if (type & ~IRQ_TYPE_SENSE_MASK) > return -EINVAL; > > spin_lock_irqsave(&gc->bgpio_lock, flags); > @@ -351,12 +350,7 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset, > > static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) > { > - u32 worked; > - struct dwapb_gpio *gpio = dev_id; > - > - worked = dwapb_do_irq(gpio); > - > - return worked ? IRQ_HANDLED : IRQ_NONE; > + return IRQ_RETVAL(dwapb_do_irq(dev_id)); > } BTW Forgot to mention. Irrelevantly to this patch just so you know seeing you are from Intel and this part is being utilized by the Intel Quark SoC. dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch is applied and the FULL-RT scheduler is enabled all interrupt handlers specified by request_irq()-based methods will be handled by a kernel thread, while generic_handle_irq() is supposed to be called from the atomic context only (with interrupts disabled). As a result an ugly stack dump will be printed to the kernel log by the next code: https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152 A way to fix this is described in Documentation/driver-api/gpio/driver.rst Regards, -Sergey > > static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > -- > 2.25.1 >
On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote: > On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote: > > IRQ core provides macros such as IRQ_RETVAL(). > > Convert code to use them. > BTW Forgot to mention. Irrelevantly to this patch just so you know seeing > you are from Intel and this part is being utilized by the Intel Quark SoC. > dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel > (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch > is applied and the FULL-RT scheduler is enabled all interrupt handlers > specified by request_irq()-based methods will be handled by a kernel thread, > while generic_handle_irq() is supposed to be called from the atomic context > only (with interrupts disabled). As a result an ugly stack dump will be printed > to the kernel log by the next code: > https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152 > > A way to fix this is described in Documentation/driver-api/gpio/driver.rst There is patch from Siemens to fix that [1]. I dunno if they are going to upstream it. Jan? [1]: https://github.com/siemens/meta-iot2000/blob/master/meta-iot2000-bsp/recipes-kernel/linux/patches/rt-0002-gpio-dwapb-Work-around-RT-full-s-enforced-IRQ-thread.patch
On Thu, Apr 16, 2020 at 01:39:11PM +0300, Andy Shevchenko wrote: > On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote: > > On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote: > > > IRQ core provides macros such as IRQ_RETVAL(). > > > Convert code to use them. > > > BTW Forgot to mention. Irrelevantly to this patch just so you know seeing > > you are from Intel and this part is being utilized by the Intel Quark SoC. > > dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel > > (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch > > is applied and the FULL-RT scheduler is enabled all interrupt handlers > > specified by request_irq()-based methods will be handled by a kernel thread, > > while generic_handle_irq() is supposed to be called from the atomic context > > only (with interrupts disabled). As a result an ugly stack dump will be printed > > to the kernel log by the next code: > > https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152 > > > > A way to fix this is described in Documentation/driver-api/gpio/driver.rst > > There is patch from Siemens to fix that [1]. I dunno if they are going to upstream it. > Jan? > > [1]: https://github.com/siemens/meta-iot2000/blob/master/meta-iot2000-bsp/recipes-kernel/linux/patches/rt-0002-gpio-dwapb-Work-around-RT-full-s-enforced-IRQ-thread.patch Just to note I wouldn't accept that patch as it is, because it's applied to all types of IRQ handlers supported by the driver. The chained cascaded IRQ handlers won't be converted to the kernel threads in RT-patched kernels [1], so using the wa-lock is redundant in that case. One of the ways to fix it is to have a conditional wa_lock acquisition depended on the irq_shared flag state. Alternatively we could create a dedicated handlers for each types of the IRQs. [1] Documentation/driver-api/gpio/driver.rst Regards, -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On 16.04.20 13:01, Serge Semin wrote: > On Thu, Apr 16, 2020 at 01:39:11PM +0300, Andy Shevchenko wrote: >> On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote: >>> On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote: >>>> IRQ core provides macros such as IRQ_RETVAL(). >>>> Convert code to use them. >> >>> BTW Forgot to mention. Irrelevantly to this patch just so you know seeing >>> you are from Intel and this part is being utilized by the Intel Quark SoC. >>> dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel >>> (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch >>> is applied and the FULL-RT scheduler is enabled all interrupt handlers >>> specified by request_irq()-based methods will be handled by a kernel thread, >>> while generic_handle_irq() is supposed to be called from the atomic context >>> only (with interrupts disabled). As a result an ugly stack dump will be printed >>> to the kernel log by the next code: >>> https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152 >>> >>> A way to fix this is described in Documentation/driver-api/gpio/driver.rst >> >> There is patch from Siemens to fix that [1]. I dunno if they are going to upstream it. >> Jan? >> >> [1]: https://github.com/siemens/meta-iot2000/blob/master/meta-iot2000-bsp/recipes-kernel/linux/patches/rt-0002-gpio-dwapb-Work-around-RT-full-s-enforced-IRQ-thread.patch > > Just to note I wouldn't accept that patch as it is, because it's applied to all > types of IRQ handlers supported by the driver. The chained cascaded IRQ > handlers won't be converted to the kernel threads in RT-patched kernels [1], so > using the wa-lock is redundant in that case. One of the ways to fix it is > to have a conditional wa_lock acquisition depended on the irq_shared flag > state. Alternatively we could create a dedicated handlers for each types of > the IRQs. Yeah, never had the time to dig into details. Also, that patch was only tested on one particular device with one particular kernel series (4.4-cip-rt). That's why it's a "workaround", not a fix. Jan > > [1] Documentation/driver-api/gpio/driver.rst > > Regards, > -Sergey > >> >> -- >> With Best Regards, >> Andy Shevchenko >> >>
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 8b30ded9322a..4edac592c253 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -258,8 +258,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) irq_hw_number_t bit = irqd_to_hwirq(d); unsigned long level, polarity, flags; - if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING | - IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) + if (type & ~IRQ_TYPE_SENSE_MASK) return -EINVAL; spin_lock_irqsave(&gc->bgpio_lock, flags); @@ -351,12 +350,7 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset, static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) { - u32 worked; - struct dwapb_gpio *gpio = dev_id; - - worked = dwapb_do_irq(gpio); - - return worked ? IRQ_HANDLED : IRQ_NONE; + return IRQ_RETVAL(dwapb_do_irq(dev_id)); } static void dwapb_configure_irqs(struct dwapb_gpio *gpio,