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

[Grygorii Strashko]

On 12/02/2021 15:12, Song Bao Hua (Barry Song) wrote:
> > -----Original Message-----
> > From: Grygorii Strashko [mailto:grygorii.strashko@xxxxxx]
> > Sent: Saturday, February 13, 2021 12:53 AM
> > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxxxx>; luojiaxing <luojiaxing@xxxxxxxxxx>; Linus
> > Walleij <linus.walleij@xxxxxxxxxx>; Santosh Shilimkar <ssantosh@xxxxxxxxxx>;
> > Kevin Hilman <khilman@xxxxxxxxxx>; open list:GPIO SUBSYSTEM
> > <linux-gpio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > linuxarm@xxxxxxxxxxxxx
> > 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@xxxxxxxxx]
> > > > Sent: Friday, February 12, 2021 11:57 PM
> > > > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> > > > Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>; Arnd Bergmann
> > > > <arnd@xxxxxxxxxx>; luojiaxing <luojiaxing@xxxxxxxxxx>; Linus Walleij
> > > > <linus.walleij@xxxxxxxxxx>; Santosh Shilimkar <ssantosh@xxxxxxxxxx>; Kevin
> > > > Hilman <khilman@xxxxxxxxxx>; open list:GPIO SUBSYSTEM
> > > > <linux-gpio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > > > linuxarm@xxxxxxxxxxxxx
> > > > 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@xxxxxx]
> > > > > > 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@xxxxxxxxxxxxx> 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?
> > >
> > > 1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so no
> > more
> > > interrupt on the same cpu -> No deadleak.
> > >
> > > 2. 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().

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