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

[Brad Harper]

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