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

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@xxxxxx]
> Sent: Saturday, February 13, 2021 3:09 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 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().

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