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: Friday, February 12, 2021 11:28 PM
> To: Arnd Bergmann <arnd@xxxxxxxxxx>; Song Bao Hua (Barry Song)
> <song.bao.hua@xxxxxxxxxxxxx>
> Cc: luojiaxing <luojiaxing@xxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; Andy
> Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; 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()
> 
> Hi Arnd,
> 
> 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:
> >>> -----Original Message-----
> >
> >>>
> >>> 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.

> 
> 
> 
> --
> Best regards,
> Grygorii

Thanks
Barry