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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> 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.

> 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.

Jan