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

From: Marek Vasut <marex@denx.de>
To: Johan Hovold <johan@kernel.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: 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: Thu, 12 Jan 2023 17:38:48 +0100	[thread overview]
Message-ID: <78e3f61f-4f87-9ac5-7c7c-172714da69d3@denx.de> (raw)
In-Reply-To: <Y8AHaJIttNga68q4@hovoldconsulting.com>

On 1/12/23 14:13, Johan Hovold wrote:
> 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.

The author does not have enough experience with preempt-rt to make that 
determination, hence deferred to Sebastian for better judgement.

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

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel