[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e4e6cc45-bc18-40ec-035e-fdb45b9a8f46@gmail.com>
Date: Wed, 7 Oct 2020 00:45:20 +1100
From: Brad Harper <bjharper@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>,
Ulf Hansson <ulf.hansson@...aro.org>
Cc: Jerome Brunet <jbrunet@...libre.com>,
Kevin Hilman <khilman@...libre.com>,
"open list:ARM/Amlogic Meson..." <linux-amlogic@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
I'm happy to test anything on a range of amlogic hardware with standard
/ rt and multiple mmc devices. Ill test Jerome's patch in next 24
hours to report the results.
On 6/10/2020 11:43 pm, Thomas Gleixner wrote:
> On Mon, Oct 05 2020 at 10:55, Thomas Gleixner wrote:
>> On Mon, Oct 05 2020 at 10:22, Ulf Hansson wrote:
>>> On Fri, 2 Oct 2020 at 18:49, Jerome Brunet <jbrunet@...libre.com> wrote:
>>>> IRQF_ONESHOT was added to this driver to make sure the irq was not enabled
>>>> again until the thread part of the irq had finished doing its job.
>>>>
>>>> Doing so upsets RT because, under RT, the hardirq part of the irq handler
>>>> is not migrated to a thread if the irq is claimed with IRQF_ONESHOT.
>>>> In this case, it has been reported to eventually trigger a deadlock with
>>>> the led subsystem.
>>>>
>>>> Preventing RT from doing this migration was certainly not the intent, the
>>>> description of IRQF_ONESHOT does not really reflect this constraint:
>>>>
>>>> > IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
>>>> > Used by threaded interrupts which need to keep the
>>>> > irq line disabled until the threaded handler has been run.
>>>>
>>>> This is exactly what this driver was trying to acheive so I'm still a bit
>>>> confused whether this is a driver or an RT issue.
>>>>
>>>> Anyway, this can be solved driver side by manually disabling the IRQs
>>>> instead of the relying on the IRQF_ONESHOT. IRQF_ONESHOT may then be removed
>>>> while still making sure the irq won't trigger until the threaded part of
>>>> the handler is done.
>>> Thomas, may I have your opinion on this one.
>>>
>>> I have no problem to apply $subject patch, but as Jerome also
>>> highlights above - this kind of makes me wonder if this is an RT
>>> issue, that perhaps deserves to be solved in a generic way.
>>>
>>> What do you think?
>> Let me stare at the core code. Something smells fishy.
> The point is that for threaded interrupts (without a primary handler)
> the core needs to be told that the interrupt line should be masked until
> the threaded handler finished. That's what IRQF_ONESHOT is for.
>
> For interrupts which have both a primary and a threaded handler that's a
> different story. The primary handler decides whether the thread should
> be woken and it decides whether to block further interrupt delivery in
> the device or keep it enabled.
>
> When forced interrupt threading is enabled (even independent of RT) then
> we have the following cases:
>
> 1) Regular device interrupt (primary handler only)
>
> The primary handler is replaced with the default 'wake up thread'
> handler and the original primary handler becomes the threaded
> handler. This enforces IRQF_ONESHOT so that the interupt line (for
> level interrupts) stays masked until the thread completed handling.
>
> 2) Threaded interrupts
>
> Interrupts which have been requested as threaded handler (no
> primary handler) are not changed obvioulsy
>
> 3) Interrupts which have both a primary and a thread handler
>
> Here IRQF_ONESHOT decides whether the primary handler will be
> forced threaded or not.
>
> That's a bit unfortunate and ill defined and was not intended to be
> used that way.
>
> We rather should make interrupts which need to have their primary
> handler in hard interrupt context to set IRQF_NO_THREAD. That
> should at the same time confirm that the primary handler is RT
> safe.
>
> Let me stare at the core code and the actual usage sites some more.
>
> Thanks,
>
> tglx
>
>
>
>
>
>
Powered by blists - more mailing lists