linux-kernel - Re: [PATCH] mmc: meson-gx: remove IRQF

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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
>
>
>
>
>
>