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@amd.com>
---
 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;
+}
+
 /*
  * Reroute an IRQ to a different pin.
  */
@@ -200,15 +208,12 @@ static void __init replace_pin_at_irq(unsigned int irq,
 {
     struct irq_pin_list *entry = irq_2_pin + irq;
 
-    while (1) {
+    for (; entry; entry = next_entry(entry)) {
         if (entry->apic == oldapic && entry->pin == oldpin) {
             entry->apic = newapic;
             entry->pin = newpin;
             share_vector_maps(oldapic, newapic);
         }
-        if (!entry->next)
-            break;
-        entry = irq_2_pin + entry->next;
     }
 }
 
@@ -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;
     }
 }
 
@@ -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;
         __io_apic_eoi(entry->apic, vector, pin);
-        if (!entry->next)
-            break;
-        entry = irq_2_pin + entry->next;
     }
 }
 
@@ -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;
         }
     }
 
@@ -1308,12 +1303,8 @@ static void /*__init*/ __print_IO_APIC(bool boot)
         if (entry->pin < 0)
             continue;
         printk(KERN_DEBUG "IRQ%d ", irq_to_desc(i)->arch.vector);
-        for (;;) {
+        for (; entry; entry = next_entry(entry))
             printk("-> %d:%d", entry->apic, entry->pin);
-            if (!entry->next)
-                break;
-            entry = irq_2_pin + entry->next;
-        }
         printk("\n");
     }
 
@@ -1586,14 +1577,6 @@ static int __init cf_check setup_ioapic_ack(const char *s)
 }
 custom_param("ioapic_ack", setup_ioapic_ack);
 
-static struct irq_pin_list *next_entry(const struct irq_pin_list *entry)
-{
-    if ( !entry->next )
-        return NULL;
-
-    return irq_2_pin + entry->next;
-}
-
 static bool io_apic_level_ack_pending(unsigned int irq)
 {
     struct irq_pin_list *entry;
@@ -2415,7 +2398,7 @@ void dump_ioapic_irq_info(void)
 
         printk("    IRQ%3d Vec%3d:\n", irq, irq_to_vector(irq));
 
-        for ( ; ; )
+        for ( ; entry; entry = next_entry(entry))
         {
             pin = entry->pin;
 
@@ -2432,10 +2415,6 @@ void dump_ioapic_irq_info(void)
                    (x2apic_enabled && iommu_intremap) ? 8 : 2,
                    (x2apic_enabled && iommu_intremap) ?
                        rte.dest.dest32 : rte.dest.logical.logical_dest);
-
-            if ( entry->next == 0 )
-                break;
-            entry = &irq_2_pin[entry->next];
         }
     }
 }
-- 
2.51.0

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

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

On 2025-10-16 02:53, Jan Beulich wrote:
> 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@amd.com>
>> ---
>>   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@suse.com>
> 
>> @@ -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).

Right.  I don't know.  It seems like ->pin == -1 indicates an unused 
entry, so stopping in the case makes sense.  I've wondered if 
io_apic_level_ack_pending() continues just in case an entry->next points 
to another pin to ack.  i.e. it's not the expected case, but it also 
might help if entry->next points to something valid.

Regards,
Jason