Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT

[Brad Harper <bjharper@gmail.com>]

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@baylibre.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
>
>
>
>
>
>