[v2,08/14] gpio: dwapb: Convert to use IRQ core provided macros

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(-)

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
>>
>>