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

From: Jan Kiszka <jan.kiszka@siemens.com>
To: Serge Semin <fancer.lancer@gmail.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-gpio@vger.kernel.org
Subject: Re: [PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros
Date: Thu, 16 Apr 2020 14:00:32 +0200	[thread overview]
Message-ID: <d4840691-72d6-316c-a66f-a44d422b07cb@siemens.com> (raw)
In-Reply-To: <20200416110135.dieh6gctkh3bwyiz@mobilestation>

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
>>
>>
-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux