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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

(While looking, I also notice that IRQ_REPLAY has been completely useless
for perhaps a very long time. I'll make a patch to zap that.)

Jan