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

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

On Mon, Jun 29, 2026 at 12:39:45PM +0200, Jan Beulich wrote:
> On 29.06.2026 11:45, Roger Pau Monne wrote:
> > --- a/xen/common/irq.c
> > +++ b/xen/common/irq.c
> > @@ -54,3 +54,15 @@ unsigned int cf_check irq_startup_none(struct irq_desc 
> > *desc)
> >  {
> >      return 0;
> >  }
> > +
> > +void disable_irq(unsigned int irq)
> > +{
> > +    struct irq_desc *desc = irq_to_desc(irq);
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&desc->lock, flags);
> > +    desc->status |= IRQ_DISABLED;
> > +    if ( desc->handler->disable )
> > +        desc->handler->disable(desc);
> 
> I'd like to point out that __pirq_guest_unbind() has this the other way 
> around:
> Call ->disable(), then set flag. Similarly move_native_irq() only calls the
> hook with the flag clear. Whereas fixup_irqs() doesn't care about the flag at
> all. Also considering the wording "disable" vs "disabled", I think setting the
> flag afterwards is better.

As this was done inside of the locked region I didn't think it
mattered much, but yes, the ->disable() handler might itself rely on
the previous state.

> > @@ -190,12 +191,38 @@ static void cf_check ns16550_interrupt(int irq, void 
> > *dev_id)
> >  {
> >      struct serial_port *port = dev_id;
> >      struct ns16550 *uart = port->uart;
> > +    /*
> > +     * Set quite arbitrarily as 4x the time to drain the TX or fill RX 
> > FIFOs,
> > +     * set the upper bound as 5ms or the timeout_ms value, whatever is 
> > higher.
> > +     */
> > +    const unsigned int delta = min(uart->timeout_ms * 4,
> > +                                   max(5u, uart->timeout_ms));
> 
> You may want to also update the description accordingly.
> 
> Preferably with respective adjustments:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks, I've applied the comments.

Regards, Roger.