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

[Marek Vasut]

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.

[...]