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

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Johan Hovold <johan@kernel.org>
Cc: Marek Vasut <marex@denx.de>,
	linux-serial@vger.kernel.org,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Erwan Le Ray <erwan.leray@foss.st.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Valentin Caron <valentin.caron@foss.st.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler
Date: Mon, 9 Jan 2023 11:13:15 +0100	[thread overview]
Message-ID: <Y7vou3wAeLP4X+TY@linutronix.de> (raw)
In-Reply-To: <Y6sHr5kuxUoahlzJ@hovoldconsulting.com>

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

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

> Johan

Sebastian