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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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 )

Regards, Roger.