| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8ca9814a674f4d3ab8bd74698ef9e693@hisilicon.com>
Date: Fri, 12 Feb 2021 20:06:11 +0000
From: "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To: Grygorii Strashko <grygorii.strashko@...com>,
Andy Shevchenko <andy.shevchenko@...il.com>
CC: Arnd Bergmann <arnd@...nel.org>,
luojiaxing <luojiaxing@...wei.com>,
Linus Walleij <linus.walleij@...aro.org>,
Santosh Shilimkar <ssantosh@...nel.org>,
Kevin Hilman <khilman@...nel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linuxarm@...neuler.org" <linuxarm@...neuler.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()
> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@...com]
> Sent: Saturday, February 13, 2021 3:09 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>; Andy Shevchenko
> <andy.shevchenko@...il.com>
> Cc: Arnd Bergmann <arnd@...nel.org>; luojiaxing <luojiaxing@...wei.com>; Linus
> Walleij <linus.walleij@...aro.org>; Santosh Shilimkar <ssantosh@...nel.org>;
> Kevin Hilman <khilman@...nel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@...r.kernel.org>; linux-kernel@...r.kernel.org;
> linuxarm@...neuler.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@...com]
> >> Sent: Saturday, February 13, 2021 12:53 AM
> >> To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>; Andy Shevchenko
> >> <andy.shevchenko@...il.com>
> >> Cc: Arnd Bergmann <arnd@...nel.org>; luojiaxing <luojiaxing@...wei.com>;
> Linus
> >> Walleij <linus.walleij@...aro.org>; Santosh Shilimkar
> <ssantosh@...nel.org>;
> >> Kevin Hilman <khilman@...nel.org>; open list:GPIO SUBSYSTEM
> >> <linux-gpio@...r.kernel.org>; linux-kernel@...r.kernel.org;
> >> linuxarm@...neuler.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@...il.com]
> >>>> Sent: Friday, February 12, 2021 11:57 PM
> >>>> To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>
> >>>> Cc: Grygorii Strashko <grygorii.strashko@...com>; Arnd Bergmann
> >>>> <arnd@...nel.org>; luojiaxing <luojiaxing@...wei.com>; Linus Walleij
> >>>> <linus.walleij@...aro.org>; Santosh Shilimkar <ssantosh@...nel.org>;
> Kevin
> >>>> Hilman <khilman@...nel.org>; open list:GPIO SUBSYSTEM
> >>>> <linux-gpio@...r.kernel.org>; linux-kernel@...r.kernel.org;
> >>>> linuxarm@...neuler.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@...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@...ilicon.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?
> >>>
> >>> 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
Powered by blists - more mailing lists