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

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

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.

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.

Thanks, Roger.