Message-ID: <87v9fn7ce2.fsf@nanos.tec.linutronix.de>
Date:   Tue, 06 Oct 2020 14:43:49 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     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\@vger.kernel.org" <linux-mmc@...r.kernel.org>,
        Brad Harper <bjharper@...il.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT

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