Messages in this thread | | | Subject | Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT | From | Brad Harper <> | Date | Wed, 7 Oct 2020 00:45:20 +1100 |
| |
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 > > > > > >
| |