Re: [PATCH for-4.22] char/ns16550: bound execution time of ns16550_interrupt()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2026 12:08, Roger Pau Monné wrote:
> On Wed, Jun 24, 2026 at 10:01:36AM +0200, Jan Beulich wrote:
>> On 23.06.2026 17:54, Roger Pau Monné wrote:
>>> On Tue, Jun 23, 2026 at 04:27:12PM +0200, Jan Beulich wrote:
>>>> On 23.06.2026 16:16, Roger Pau Monné wrote:
>>>>> On Tue, Jun 23, 2026 at 03:44:06PM +0200, Jan Beulich wrote:
>>>>>> On 23.06.2026 12:31, Roger Pau Monne wrote:
>>>>>>> +    if ( uart->force_polling )
>>>>>>> +        return;
>>>>>>
>>>>>> As the IRQ was disabled, is this even possible? I.e. should this be some
>>>>>> kind of assertion or alike?
>>>>>
>>>>> Hm, I wasn't setting IRQ_DISABLED before, and hence needed this guard.
>>>>> But now with IRQ_DISABLED being set in ->status do_IRQ() should filter
>>>>> any stray interrupts.  I will attempt to add an ASSERT_UNREACHABLE()
>>>>> here.
>>>>
>>>> Simply ASSERT(!uart->force_polling) should do here? It is not wrong to
>>>> run the code below in release builds in such an event. If we kept getting
>>>> interrupts (perhaps at a high frequency) we'd be in trouble anyway.
>>>
>>> No, I'm afraid I can't do it like that, I can't put an ASSERT there,
>>> because we can still get into ns16550_interrupt() after the interrupt
>>> has been disabled.  In do_IRQ() we have the following loop:
>>>
>>>     while ( desc->status & IRQ_PENDING )
>>>     {
>>>         desc->status &= ~IRQ_PENDING;
>>>         spin_unlock_irq(&desc->lock);
>>>
>>>         tsc_in = tb_init_done ? get_cycles() : 0;
>>>         action->handler(irq, action->dev_id);
>>>         TRACE_TIME(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
>>>
>>>         spin_lock_irq(&desc->lock);
>>>     }
>>>
>>> So if the device is generating further interrupts in the window with
>>> IRQs enabled (while we execute the handler), we will keep looping
>>> around this, without taking into account the setting of IRQ_DISABLED.
>>
>> Ah yes.
>>
>>> This is something that we might want to fix, so that the loop is bound
>>> by IRQ_PENDING being set, and IRQ_DISABLED not, ie:
>>>
>>>     while ( (desc->status & (IRQ_PENDING | IRQ_DISABLED)) == IRQ_PENDING )
>>
>> Or perhaps ahead of the loop
>>
>>     desc->status &= ~IRQ_REPLAY;
>>
>>     if ( desc->status & IRQ_DISABLED )
>>         goto out;
>>
>>     desc->status |= IRQ_PENDING;
>>
>>     /*
>>      * Since we set PENDING, if another processor is handling a different
>>      * instance of this same irq, the other processor will take care of it.
>>      */
>>     if ( desc->status & IRQ_INPROGRESS )
>>         goto out;
>>
>>     desc->status |= IRQ_INPROGRESS;
>>
>> thus also having the comment no longer describe only part of the conditional.
> 
> I think this is racy.  An interrupt hitting in the window with
> interrupts enabled ahead of the handler having set IRQ_DISABLED will
> still set IRQ_PENDING, and thus the loop would get executed a further
> time, and the handler called after IRQ_DISABLED having been set.

Hmm, I don't quite agree with how you put it, but I think I see what you mean.
There's one question here, though: If PENDING is set first, and DISABLED only
later, shouldn't that IRQ instance still be handled? If so, ...

> I think we need an extra condition in the loop, I see no way this can
> be solved only by dealing with the concurrent setting of IRQ_PENDING.

... such an extra condition would be wrong. If not, yes, I agree.

Jan