[PATCH for-4.22 v2 0/4] ns16550: bound interrupt handler execution time

[PATCH for-4.22 v2 0/4] ns16550: bound interrupt handler execution time

[Roger Pau Monne]

Hello,

Same approach as v1, but the introduction of a generic disable_irq()
handler requires two pre-patches for PowerPC and RISCV.  There's also an
extra fix for dealing with the interrupt being disabled while executing
the handler in do_IRQ().

Thanks, Roger.

Roger Pau Monne (4):
  riscv/irq: define a per-arch irq_to_desc()
  xen/ppc: introduce a dummy irq_to_desc()
  xen/irq: handle IRQ being disabled while executing its handler
  char/ns16550: bound execution time of ns16550_interrupt()

 xen/arch/ppc/include/asm/irq.h   |  6 ++++++
 xen/arch/riscv/include/asm/irq.h |  4 ++++
 xen/arch/riscv/irq.c             |  5 +++++
 xen/arch/x86/irq.c               |  4 +++-
 xen/common/irq.c                 | 12 ++++++++++++
 xen/drivers/char/ns16550.c       | 30 +++++++++++++++++++++++++++++-
 xen/include/xen/irq.h            |  1 +
 7 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH for-4.22 v2 1/4] riscv/irq: define a per-arch irq_to_desc()

[Roger Pau Monne]

RISCV declares irq_desc as a static array, opposed to x86 that uses a
pointer allocated at boot time.  This creates issues when attempting to add
an extern declaration for irq_desc, as asm/irq.h is included by xen/irq.h where
the definition of struct irq_desc resides, and an empty forward declaration
doesn't make the compiler happy because it doesn't know the type
data-storage.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/riscv/include/asm/irq.h | 4 ++++
 xen/arch/riscv/irq.c             | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index f633636dc308..b3e03117ac97 100644
--- a/xen/arch/riscv/include/asm/irq.h
+++ b/xen/arch/riscv/include/asm/irq.h
@@ -35,6 +35,10 @@ struct arch_irq_desc {
     unsigned int type;
 };
 
+struct irq_desc;
+struct irq_desc *irq_to_desc(unsigned int irq);
+#define irq_to_desc irq_to_desc
+
 struct cpu_user_regs;
 struct dt_device_node;
 
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 25d329500212..353e9246f15b 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -19,6 +19,11 @@
 
 static irq_desc_t irq_desc[NR_IRQS];
 
+struct irq_desc *irq_to_desc(unsigned int irq)
+{
+    return &irq_desc[irq];
+}
+
 static bool irq_validate_new_type(unsigned int curr, unsigned int new)
 {
     return curr == IRQ_TYPE_INVALID || curr == new;
-- 
2.53.0

Re: [PATCH for-4.22 v2 1/4] riscv/irq: define a per-arch irq_to_desc()

[Oleksii Kurochko]

On 6/29/26 11:45 AM, Roger Pau Monne wrote:
> RISCV declares irq_desc as a static array, opposed to x86 that uses a
> pointer allocated at boot time.  This creates issues when attempting to add
> an extern declaration for irq_desc, as asm/irq.h is included by xen/irq.h where
> the definition of struct irq_desc resides, and an empty forward declaration
> doesn't make the compiler happy because it doesn't know the type
> data-storage.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/arch/riscv/include/asm/irq.h | 4 ++++
>   xen/arch/riscv/irq.c             | 5 +++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
> index f633636dc308..b3e03117ac97 100644
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -35,6 +35,10 @@ struct arch_irq_desc {
>       unsigned int type;
>   };
>   
> +struct irq_desc;
> +struct irq_desc *irq_to_desc(unsigned int irq);
> +#define irq_to_desc irq_to_desc
> +
>   struct cpu_user_regs;
>   struct dt_device_node;
>   
> diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
> index 25d329500212..353e9246f15b 100644
> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -19,6 +19,11 @@
>   
>   static irq_desc_t irq_desc[NR_IRQS];
>   
> +struct irq_desc *irq_to_desc(unsigned int irq)
> +{
> +    return &irq_desc[irq];
> +}
> +
>   static bool irq_validate_new_type(unsigned int curr, unsigned int new)
>   {
>       return curr == IRQ_TYPE_INVALID || curr == new;

Changes looks good to me.

There are similar changes in my dom0less enablement config for RISC-V 
but considering that likely this patch will be in staging faster then my 
patch series:
  Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
  Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

Re: [PATCH for-4.22 v2 1/4] riscv/irq: define a per-arch irq_to_desc()

[Jan Beulich]

On 29.06.2026 11:45, Roger Pau Monne wrote:
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -35,6 +35,10 @@ struct arch_irq_desc {
>      unsigned int type;
>  };
>  
> +struct irq_desc;

Nit: You don't need this here, as ...

> +struct irq_desc *irq_to_desc(unsigned int irq);

... it's the function return type, not a parameter one.

> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -19,6 +19,11 @@
>  
>  static irq_desc_t irq_desc[NR_IRQS];
>  
> +struct irq_desc *irq_to_desc(unsigned int irq)
> +{
> +    return &irq_desc[irq];
> +}

Maybe better assert that the incoming number is in range? By this not
(possibly, in the current arrangements) being an inline function, the
compiler can't assist.

Jan

[PATCH for-4.22 v2 2/4] xen/ppc: introduce a dummy irq_to_desc()

[Roger Pau Monne]

In preparation for irq_to_desc() being called by common IRQ code.
PowerPC doesn't have an irq_desc array defined, so it cannot use the
generic irq_to_desc macro in the common header.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/ppc/include/asm/irq.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/ppc/include/asm/irq.h b/xen/arch/ppc/include/asm/irq.h
index 5c37d0cf2500..fa70fd7d46dc 100644
--- a/xen/arch/ppc/include/asm/irq.h
+++ b/xen/arch/ppc/include/asm/irq.h
@@ -30,4 +30,10 @@ static inline int platform_get_irq(const struct dt_device_node *device, int inde
     BUG_ON("unimplemented");
 }
 
+static inline void *irq_to_desc(unsigned int irq)
+{
+    BUG_ON("unimplemented");
+}
+#define irq_to_desc irq_to_desc
+
 #endif /* __ASM_PPC_IRQ_H__ */
-- 
2.53.0

Re: [PATCH for-4.22 v2 2/4] xen/ppc: introduce a dummy irq_to_desc()

[Jan Beulich]

On 29.06.2026 11:45, Roger Pau Monne wrote:
> In preparation for irq_to_desc() being called by common IRQ code.
> PowerPC doesn't have an irq_desc array defined, so it cannot use the
> generic irq_to_desc macro in the common header.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH for-4.22 v2 2/4] xen/ppc: introduce a dummy irq_to_desc()

[Oleksii Kurochko]

On 6/29/26 11:45 AM, Roger Pau Monne wrote:
> In preparation for irq_to_desc() being called by common IRQ code.
> PowerPC doesn't have an irq_desc array defined, so it cannot use the
> generic irq_to_desc macro in the common header.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/arch/ppc/include/asm/irq.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/ppc/include/asm/irq.h b/xen/arch/ppc/include/asm/irq.h
> index 5c37d0cf2500..fa70fd7d46dc 100644
> --- a/xen/arch/ppc/include/asm/irq.h
> +++ b/xen/arch/ppc/include/asm/irq.h
> @@ -30,4 +30,10 @@ static inline int platform_get_irq(const struct dt_device_node *device, int inde
>       BUG_ON("unimplemented");
>   }
>   
> +static inline void *irq_to_desc(unsigned int irq)
> +{
> +    BUG_ON("unimplemented");
> +}
> +#define irq_to_desc irq_to_desc
> +
>   #endif /* __ASM_PPC_IRQ_H__ */

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

Re: [PATCH for-4.22 v2 2/4] xen/ppc: introduce a dummy irq_to_desc()

[Oleksii Kurochko]

On 6/29/26 12:09 PM, Oleksii Kurochko wrote:
> 
> 
> On 6/29/26 11:45 AM, Roger Pau Monne wrote:
>> In preparation for irq_to_desc() being called by common IRQ code.
>> PowerPC doesn't have an irq_desc array defined, so it cannot use the
>> generic irq_to_desc macro in the common header.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>   xen/arch/ppc/include/asm/irq.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/xen/arch/ppc/include/asm/irq.h b/xen/arch/ppc/include/ 
>> asm/irq.h
>> index 5c37d0cf2500..fa70fd7d46dc 100644
>> --- a/xen/arch/ppc/include/asm/irq.h
>> +++ b/xen/arch/ppc/include/asm/irq.h
>> @@ -30,4 +30,10 @@ static inline int platform_get_irq(const struct 
>> dt_device_node *device, int inde
>>       BUG_ON("unimplemented");
>>   }
>> +static inline void *irq_to_desc(unsigned int irq)
>> +{
>> +    BUG_ON("unimplemented");
>> +}
>> +#define irq_to_desc irq_to_desc
>> +
>>   #endif /* __ASM_PPC_IRQ_H__ */
> 
> Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

[PATCH for-4.22 v2 3/4] xen/irq: handle IRQ being disabled while executing its handler

[Roger Pau Monne]

It's possible for IRQ_DISABLED to be set while running the handler
execution loop in do_IRQ().  Such disabling can be done by the handler
itself, or from a remote CPU.

Check for IRQ_DISABLED not being set in the loop condition, as to not
execute the handler if the IRQ has been disabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I wanted to add a fixes tag, but this has been broken since forever, and
hence I should add:

Fixes: 4676bbf96dc8 ("bitkeeper revision 1.2 (3ddb79c9KusG02eh7i-uXkgY0IksKA)")

Which is pointless IMO.
---
 xen/arch/x86/irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 739fc04bd16a..2082ad489d74 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2088,7 +2088,9 @@ void do_IRQ(struct cpu_user_regs *regs)
     desc->status |= IRQ_INPROGRESS;
 
     action = desc->action;
-    while ( desc->status & IRQ_PENDING )
+
+    /* Deal with IRQ_DISABLED being set while inside the loop body. */
+    while ( (desc->status & (IRQ_PENDING | IRQ_DISABLED)) == IRQ_PENDING )
     {
         desc->status &= ~IRQ_PENDING;
         spin_unlock_irq(&desc->lock);
-- 
2.53.0

Re: [PATCH for-4.22 v2 3/4] xen/irq: handle IRQ being disabled while executing its handler

[Jan Beulich]

On 29.06.2026 11:45, Roger Pau Monne wrote:
> It's possible for IRQ_DISABLED to be set while running the handler
> execution loop in do_IRQ().  Such disabling can be done by the handler
> itself, or from a remote CPU.

I don't think the handler itself can legitimately call any of the functions
setting IRQ_DISABLED.

> Check for IRQ_DISABLED not being set in the loop condition, as to not
> execute the handler if the IRQ has been disabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Preferably with the description adjusted (or it being pointed out what I may
be overlooking):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> I wanted to add a fixes tag, but this has been broken since forever, and
> hence I should add:
> 
> Fixes: 4676bbf96dc8 ("bitkeeper revision 1.2 (3ddb79c9KusG02eh7i-uXkgY0IksKA)")
> 
> Which is pointless IMO.

+1

Jan

Re: [PATCH for-4.22 v2 3/4] xen/irq: handle IRQ being disabled while executing its handler

[Roger Pau Monné]

On Mon, Jun 29, 2026 at 12:31:56PM +0200, Jan Beulich wrote:
> On 29.06.2026 11:45, Roger Pau Monne wrote:
> > It's possible for IRQ_DISABLED to be set while running the handler
> > execution loop in do_IRQ().  Such disabling can be done by the handler
> > itself, or from a remote CPU.
> 
> I don't think the handler itself can legitimately call any of the functions
> setting IRQ_DISABLED.

Hm, yes, we still have no interrupt handlers that set IRQ_DISABLED,
but we will gain one in the next patch, where ns16550_interrupt() will
call disable_irq() that does set IRQ_DISABLED.

> > Check for IRQ_DISABLED not being set in the loop condition, as to not
> > execute the handler if the IRQ has been disabled.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Preferably with the description adjusted (or it being pointed out what I may
> be overlooking):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm happy to clarify the commit message to note that while we have no
instances ATM, but that future changes might introduce some.

Thanks, Roger.

Re: [PATCH for-4.22 v2 3/4] xen/irq: handle IRQ being disabled while executing its handler

[Jan Beulich]

On 29.06.2026 16:29, Roger Pau Monné wrote:
> On Mon, Jun 29, 2026 at 12:31:56PM +0200, Jan Beulich wrote:
>> On 29.06.2026 11:45, Roger Pau Monne wrote:
>>> It's possible for IRQ_DISABLED to be set while running the handler
>>> execution loop in do_IRQ().  Such disabling can be done by the handler
>>> itself, or from a remote CPU.
>>
>> I don't think the handler itself can legitimately call any of the functions
>> setting IRQ_DISABLED.
> 
> Hm, yes, we still have no interrupt handlers that set IRQ_DISABLED,
> but we will gain one in the next patch, where ns16550_interrupt() will
> call disable_irq() that does set IRQ_DISABLED.
> 
>>> Check for IRQ_DISABLED not being set in the loop condition, as to not
>>> execute the handler if the IRQ has been disabled.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Preferably with the description adjusted (or it being pointed out what I may
>> be overlooking):
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm happy to clarify the commit message to note that while we have no
> instances ATM, but that future changes might introduce some.

Better leave as is.

Jan

Re: [PATCH for-4.22 v2 3/4] xen/irq: handle IRQ being disabled while executing its handler

[Oleksii Kurochko]

On 6/29/26 11:45 AM, Roger Pau Monne wrote:
> It's possible for IRQ_DISABLED to be set while running the handler
> execution loop in do_IRQ().  Such disabling can be done by the handler
> itself, or from a remote CPU.
> 
> Check for IRQ_DISABLED not being set in the loop condition, as to not
> execute the handler if the IRQ has been disabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I wanted to add a fixes tag, but this has been broken since forever, and
> hence I should add:
> 
> Fixes: 4676bbf96dc8 ("bitkeeper revision 1.2 (3ddb79c9KusG02eh7i-uXkgY0IksKA)")
> 
> Which is pointless IMO.
> ---

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

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

[Roger Pau Monne]

The current logic in ns16550_interrupt() will loop until the device sets
the NOINT in IIR.  At least on the Lenovo ThinkSystem SR630 V4 the flow
control of the serial-over-lan emulated UART seems to be broken, as it
doesn't set the NOINT bit consistently.  The Transmitter Holding Register
Empty in LSR also seems to not be properly signaled, as even with it set
writes to the transmit register take ~6ms.  This leads to the watchdog
triggering very easily on such system.

Introduce an upper bound on the execution time of ns16550_interrupt(), this
is currently set as 4x the polling interval, which is calculated as the
time to fill RX FIFO and/or empty TX FIFO.  The current maximum is 5ms.
Once the timeout triggers the interrupt is disabled and the uart is
switched to polling mode.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Move irq disabling to its own helper.
 - Turn force_polling check in interrupt handler into an ASSERT().
 - Improve logic for timeout calculation.
---
 xen/common/irq.c           | 12 ++++++++++++
 xen/drivers/char/ns16550.c | 30 +++++++++++++++++++++++++++++-
 xen/include/xen/irq.h      |  1 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/xen/common/irq.c b/xen/common/irq.c
index 29729349a6f2..102974d120f4 100644
--- 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);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 878da27f2ef8..a371bc5cc85e 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -62,6 +62,7 @@ static struct ns16550 {
 #endif
     unsigned int timeout_ms;
     bool intr_works;
+    bool force_polling;
     bool dw_usr_bsy;
 #ifdef NS16550_PCI
     /* PCI card parameters. */
@@ -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));
+    const s_time_t timeout = NOW() + MILLISECS(delta);
 
+    ASSERT(!uart->force_polling);
     uart->intr_works = 1;
 
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
         u8 lsr = ns_read_reg(uart, UART_LSR);
+        s_time_t now = NOW();
+
+        /* Break out of the loop if spending too much time. */
+        if ( now > timeout )
+        {
+            /* Disable the interrupt source - it's never shared. */
+            disable_irq(irq);
+
+            /* Disable interrupt generation on the device and arm the timer. */
+            uart->force_polling = true;
+            ns_write_reg(uart, UART_IER, 0);
+            set_timer(&uart->timer, now + MILLISECS(uart->timeout_ms));
+            printk(XENLOG_WARNING
+                   "uart interrupt taking more than %ums, switched to polling\n",
+                   delta);
+
+            return;
+        }
 
         if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
             serial_tx_interrupt(port);
@@ -223,7 +250,7 @@ static void cf_check __ns16550_poll(const struct cpu_user_regs *regs)
     struct ns16550 *uart = port->uart;
     const struct cpu_user_regs *old_regs;
 
-    if ( uart->intr_works )
+    if ( uart->intr_works && !uart->force_polling )
         return; /* Interrupts work - no more polling */
 
     /* Mimic interrupt context. */
@@ -313,6 +340,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
     unsigned int  divisor;
 
     uart->intr_works = 0;
+    uart->force_polling = false;
 
     pci_serial_early_init(uart);
 
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 6071b00f621e..64a25c96a4b7 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -121,6 +121,7 @@ extern void release_irq(unsigned int irq, const void *dev_id);
 extern int request_irq(unsigned int irq, unsigned int irqflags,
                void (*handler)(int irq, void *dev_id),
                const char *devname, void *dev_id);
+void disable_irq(unsigned int irq);
 
 extern const hw_irq_controller no_irq_type;
 void cf_check no_action(int cpl, void *dev_id);
-- 
2.53.0

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

[Jan Beulich]

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.

> @@ -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@suse.com>

Jan

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

[Roger Pau Monné]

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@suse.com>

Thanks, I've applied the comments.

Regards, Roger.

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

[Oleksii Kurochko]

On 6/29/26 11:45 AM, Roger Pau Monne wrote:
> The current logic in ns16550_interrupt() will loop until the device sets
> the NOINT in IIR.  At least on the Lenovo ThinkSystem SR630 V4 the flow
> control of the serial-over-lan emulated UART seems to be broken, as it
> doesn't set the NOINT bit consistently.  The Transmitter Holding Register
> Empty in LSR also seems to not be properly signaled, as even with it set
> writes to the transmit register take ~6ms.  This leads to the watchdog
> triggering very easily on such system.
> 
> Introduce an upper bound on the execution time of ns16550_interrupt(), this
> is currently set as 4x the polling interval, which is calculated as the
> time to fill RX FIFO and/or empty TX FIFO.  The current maximum is 5ms.
> Once the timeout triggers the interrupt is disabled and the uart is
> switched to polling mode.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>


Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii