Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold <johan@kernel.org>]

On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote:
> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
> > On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> > > Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> > > in the hard-IRQ context even in the force-threaded mode. The
> > > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> > > sleeping locks (spinlock_t) in hard-IRQ context. This combination
> > > makes it impossible and leads to "sleeping while atomic" warnings.
> > > 
> > > Use one interrupt handler for both handlers (primary and secondary)
> > > and drop the IRQF_ONESHOT flag which is not needed.
> > > 
> > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> > 
> > I don't think a Fixes tag is warranted as this is only needed due to
> > this undocumented quirk of PREEMPT_RT.
> 
> It is not an undocumented quirk of PREEMPT_RT. The part that might not
> be well documented is that IRQF_ONESHOT won't run the primary handler as
> a threaded handler. This is also the case for IRQF_NO_THREAD and
> IRQF_PERCPU but might be more obvious.

Yeah, that not being documented seems like an oversight as generally
drivers should not need be changed to continue working on PREEMPT_RT (or
with forced-threading generally).

> Anyway, if the primary handler is not threaded then it runs in HARDIRQ
> context and here you must not use a spinlock_t. This is documented in
> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
> for this on !RT which is off by default because it triggers with printk
> (and this is worked on).

All interrupt handlers typically run in hard interrupt context unless
explicitly requested to be threaded on !RT and drivers still use
spinlock_t for that so not sure how that lockdep warning is supposed to
work. Or do you mean that you have a lockdep warning specifically for
IRQF_ONESHOT primary handlers?
 
> > And this should not be backported in any case.
> 
> Such things have been backported via -stable in the past. If you
> disagree, please keep me in loop while is merged so I can poke people to
> backport it as part of the RT patch for the relevant kernels.

The author did not seem to think this was stable material as there is no
cc-stable tag and it also seems fairly intrusive.

But if the ST guys or whoever cares about this driver are fine with this
being backported, I don't really mind either.

Johan