Re: [PATCH for-4.22 v3 2/2] x86/io_apic: Use next_entry() in loops

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2025 23:04, Jason Andryuk wrote:
> io_apic.c has a lot of ad-hoc for(;;) and while(1) loops for iterating
> over irq_pin_list entries.  Replace them with a standardized
> for loop using next_entry() to advance entry.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
>  xen/arch/x86/io_apic.c | 49 ++++++++++++------------------------------
>  1 file changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index c35d611ecf..73b48a9cb8 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -191,6 +191,14 @@ static void remove_pin_from_irq(unsigned int irq, int 
> apic, int pin)
>      irq_2_pin_free_entry = entry - irq_2_pin;
>  }
>  
> +static struct irq_pin_list *next_entry(const struct irq_pin_list *entry)
> +{
> +    if ( !entry->next )
> +        return NULL;
> +
> +    return irq_2_pin + entry->next;
> +}

Preferably with the function put in its final place right in patch 1:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -482,7 +487,7 @@ static void modify_IO_APIC_irq(unsigned int irq, unsigned 
> int enable,
>  {
>      struct irq_pin_list *entry = irq_2_pin + irq;
>  
> -    for (;;) {
> +    for (; entry; entry = next_entry(entry)) {
>          unsigned int pin = entry->pin;
>          struct IO_APIC_route_entry rte;
>  
> @@ -492,9 +497,6 @@ static void modify_IO_APIC_irq(unsigned int irq, unsigned 
> int enable,
>          rte.raw &= ~(uint64_t)disable;
>          rte.raw |= enable;
>          __ioapic_write_entry(entry->apic, pin, false, rte);
> -        if (!entry->next)
> -            break;
> -        entry = irq_2_pin + entry->next;
>      }
>  }

I notice that within here there's also a "break" upon ->pin being -1.
Seeing that io_apic_level_ack_pending() has continue there, I think we
will want to be consistent. Which way isn't quite clear to me (yet).

> @@ -545,14 +547,11 @@ static void __eoi_IO_APIC_irq(struct irq_desc *desc)
>      struct irq_pin_list *entry = irq_2_pin + desc->irq;
>      unsigned int pin, vector = desc->arch.vector;
>  
> -    for (;;) {
> +    for (; entry; entry = next_entry(entry)) {
>          pin = entry->pin;
>          if (pin == -1)
>              break;

Same here.

> @@ -632,7 +631,7 @@ set_ioapic_affinity_irq(struct irq_desc *desc, const 
> cpumask_t *mask)
>          if ( !iommu_intremap || !x2apic_enabled )
>              dest = SET_APIC_LOGICAL_ID(dest);
>          entry = irq_2_pin + irq;
> -        for (;;) {
> +        for (; entry; entry = next_entry(entry)) {
>              struct IO_APIC_route_entry rte;
>  
>              pin = entry->pin;
> @@ -643,10 +642,6 @@ set_ioapic_affinity_irq(struct irq_desc *desc, const 
> cpumask_t *mask)
>              rte.dest.dest32 = dest;
>              rte.vector = desc->arch.vector;
>              __ioapic_write_entry(entry->apic, pin, false, rte);
> -
> -            if (!entry->next)
> -                break;
> -            entry = irq_2_pin + entry->next;
>          }
>      }

And here.

Jan