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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jun 25, 2026 at 04:32:09PM +0200, Jan Beulich wrote:
> On 25.06.2026 15:07, Roger Pau Monné wrote:
> > 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;
> 
> Well, see the adjusted flow I did suggest earlier (still in context above).

But that flow above could still set PENDING ahead of the handler
having set DISABLED, and hence won't avoid at least an extra handler
call despite the IRQ having been disabled?  IOW: I think it's
partially racy w.r.t setting DISABLED in the handler.

> > 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.
> 
> Yet this can be of use for a corner case only anyway, as we set PENDING only
> after having called ->ack(). That is, after setting PENDING _another_ IRQ
> has to fire. Which is possible, but likely can be dealt with differently.

Well, if the ->disable handler wasn't a no-op for edge triggered
interrupts it wouldn't require this weird logic.  However then IRQ
migration will likely involve a mask/unmask for edge triggered
interrupts, which we want to avoid in general.  I would need to think
about it.

> > 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.
> 
> I definitely agree here. So perhaps indeed best to go with what you did
> proposed.

Thanks, let me see if I can sort out a minimally invasive solution.

Regards, Roger.