Messages in this thread | | | 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() | From | Grygorii Strashko <> | Date | Fri, 12 Feb 2021 16:08:40 +0200 |
| |
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 <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 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? >>> >>> 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
| |