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

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

On Thu, Jun 25, 2026 at 01:31:26PM +0200, Jan Beulich wrote:
> 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.

But PENDING is always set, regardless of whether the IRQ is disabled,
the normal flow in do_IRQ() is:

    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_DISABLED | IRQ_INPROGRESS) )
        goto out;

I think it's valid to have both PENDING and DISABLED set with the
current logic.  In fact, the code in ack_edge_ioapic_irq() relies on
having both PENDING and DISABLED set to mask the source, as the
->disable hook for edge triggered IO-APIC pins is a no-op.

We could likely change all this to be more straight forward, but as
with the serial interrupt handling I would rather not do that change
during a code freeze.

Thanks, Roger.