-----Original Message----- From: Grygorii Strashko [mailto:grygorii.strashko@ti.com] Sent: Saturday, February 13, 2021 3:09 AM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com; Andy Shevchenko andy.shevchenko@gmail.com Cc: Arnd Bergmann arnd@kernel.org; luojiaxing luojiaxing@huawei.com; Linus Walleij linus.walleij@linaro.org; Santosh Shilimkar ssantosh@kernel.org; Kevin Hilman khilman@kernel.org; open list:GPIO SUBSYSTEM linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linuxarm@openeuler.org Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
On 12/02/2021 15:12, Song Bao Hua (Barry Song) wrote:
-----Original Message----- From: Grygorii Strashko [mailto:grygorii.strashko@ti.com] Sent: Saturday, February 13, 2021 12:53 AM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com; Andy Shevchenko andy.shevchenko@gmail.com Cc: Arnd Bergmann arnd@kernel.org; luojiaxing luojiaxing@huawei.com;
Linus
Walleij linus.walleij@linaro.org; Santosh Shilimkar
Kevin Hilman khilman@kernel.org; open list:GPIO SUBSYSTEM linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linuxarm@openeuler.org Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
On 12/02/2021 13:29, Song Bao Hua (Barry Song) wrote:
-----Original Message----- From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] Sent: Friday, February 12, 2021 11:57 PM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com Cc: Grygorii Strashko grygorii.strashko@ti.com; Arnd Bergmann arnd@kernel.org; luojiaxing luojiaxing@huawei.com; Linus Walleij linus.walleij@linaro.org; Santosh Shilimkar ssantosh@kernel.org;
Kevin
Hilman khilman@kernel.org; open list:GPIO SUBSYSTEM linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linuxarm@openeuler.org Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com] > Sent: Friday, February 12, 2021 11:28 PM > On 12/02/2021 11:45, Arnd Bergmann wrote: >> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song) >> song.bao.hua@hisilicon.com wrote:
>>>> Note. there is also generic_handle_irq() call inside. >>> >>> So generic_handle_irq() is not safe to run in thread thus requires >>> an interrupt-disabled environment to run? If so, I'd rather this >>> irqsave moved into generic_handle_irq() rather than asking everyone >>> calling it to do irqsave. >> >> In a preempt-rt kernel, interrupts are run in task context, so they
clearly
>> should not be called with interrupts disabled, that would defeat the >> purpose of making them preemptible. >> >> generic_handle_irq() does need to run with in_irq()==true though, >> but this should be set by the caller of the gpiochip's handler, and >> it is not set by raw_spin_lock_irqsave(). > > It will produce warning from __handle_irq_event_percpu(), as this is
IRQ
> dispatcher > and generic_handle_irq() will call one of handle_level_irq or
handle_edge_irq.
> > The history behind this is commit 450fa54cfd66 ("gpio: omap: convert
to
use
> generic irq handler"). > > The resent related discussion: > https://lkml.org/lkml/2020/12/5/208
Ok, second thought. irqsave before generic_handle_irq() won't defeat the purpose of preemption too much as the dispatched irq handlers by gpiochip will run in their own threads but not in the thread of gpiochip's handler.
so looks like this patch can improve by:
- move other raw_spin_lock_irqsave to raw_spin_lock;
- keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
the warning in genirq.
Isn't the idea of irqsave is to prevent dead lock from the process context
when
we get interrupt on the *same* CPU?
Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher driver is almost always correct.
But for gpiochip, would the below be true though it is almost alway true for non-irq dispatcher?
- While gpiochip's handler runs in hardIRQ, interrupts are disabled, so
no
more
interrupt on the same cpu -> No deadleak.
- While gpiochip's handler runs in threads
- other non-threaded interrupts such as timer tick might come on same cpu,
but they are an irrelevant driver and thus they are not going to get the lock gpiochip's handler has held. -> no deadlock.
- other devices attached to this gpiochip might get interrupts, since
gpiochip's handler is running in threads, raw_spin_lock can help avoid messing up the critical data by two threads -> still no deadlock.
The worst RT case I can imagine is when gpio API is still called from hard
IRQ
context by some other device driver - some toggling for example. Note. RT or "threadirqs" does not mean gpiochip become sleepable.
In this case: threaded handler raw_spin_lock IRQ from other device hard_irq handler gpiod_x() raw_spin_lock_irqsave() -- oops
Actually no oops here. other drivers don't hold the same spinlock of this driver.
huh. driver/module A requests gpio and uses it in its hard_irq handler by calling GPIO API (Like gpiod_set_value()), those will go to this driver and end up in omap_gpio_set().
Yes, this could be a corner though it doesn't make any sense to use IRQF_NO_THREAD for this kind of driver/module A on rt as this will defeat the purpose of preemption by adding a long irqsoff section.
Since it cannot completely avoid this lockdep issue, I think that it is pointless to continue struggling with this patch which is changing an irq dispatcher driver any more.
But in general, what are the benefit of such changes at all, except better
marking
call context annotation, so we are spending so much time on it?
TBH, the benefit is really tiny except code cleanup. just curious how things
could
be different while it happens in an irq dispatcher's handler.
-- Best regards, Grygorii
Thanks Barry