io_apic_level_ack_pending() will end up in an infinite loop if
entry->pin == -1.  entry does not change, so it will keep reading -1.

Convert to a proper for loop so that continue works.  Add a new helper,
next_entry(), to handle advancing to the next irq_pin_list entry.

Noticed during code inspection.  The infinite loop was not observed.

Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
v3:
const on next_entry() parameter
Remove spaces inside for loop braces
Remove inner if (!entry) check
Expand commit message to state noticed during code inspection

v2:
continue (not break) for pin == -1.

I added the next_entry() helper since putting the expression in the for
loop is a little cluttered.  The helper can also be re-used for other
instances within the file.
---
 xen/arch/x86/io_apic.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index c384f10c1b..c35d611ecf 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1586,20 +1586,24 @@ 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;
     unsigned long flags;
 
     spin_lock_irqsave(&ioapic_lock, flags);
-    entry = &irq_2_pin[irq];
-    for (;;) {
+    for (entry = &irq_2_pin[irq]; entry; entry = next_entry(entry)) {
         unsigned int reg;
         int pin;
 
-        if (!entry)
-            break;
-
         pin = entry->pin;
         if (pin == -1)
             continue;
@@ -1609,9 +1613,6 @@ static bool io_apic_level_ack_pending(unsigned int irq)
             spin_unlock_irqrestore(&ioapic_lock, flags);
             return 1;
         }
-        if (!entry->next)
-            break;
-        entry = irq_2_pin + entry->next;
     }
     spin_unlock_irqrestore(&ioapic_lock, flags);
 
-- 
2.51.0

On 15.10.2025 23:04, Jason Andryuk wrote:
> io_apic_level_ack_pending() will end up in an infinite loop if
> entry->pin == -1.  entry does not change, so it will keep reading -1.
> 
> Convert to a proper for loop so that continue works.  Add a new helper,
> next_entry(), to handle advancing to the next irq_pin_list entry.
> 
> Noticed during code inspection.  The infinite loop was not observed.
> 
> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> v3:
> const on next_entry() parameter
> Remove spaces inside for loop braces
> Remove inner if (!entry) check
> Expand commit message to state noticed during code inspection
> 
> v2:
> continue (not break) for pin == -1.
> 
> I added the next_entry() helper since putting the expression in the for
> loop is a little cluttered.  The helper can also be re-used for other
> instances within the file.
> ---
>  xen/arch/x86/io_apic.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index c384f10c1b..c35d611ecf 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1586,20 +1586,24 @@ 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;
> +}

When replying to the v2 thread I hadn't spotted yet that a v3 was already
posted. As indicated, imo this name to too generic (now). I'd be happy to
make adjustments while committing, as long as we can agree on some less
generic name.

Jan

On 2025-10-16 02:47, Jan Beulich wrote:
> On 15.10.2025 23:04, Jason Andryuk wrote:
>> io_apic_level_ack_pending() will end up in an infinite loop if
>> entry->pin == -1.  entry does not change, so it will keep reading -1.
>>
>> Convert to a proper for loop so that continue works.  Add a new helper,
>> next_entry(), to handle advancing to the next irq_pin_list entry.
>>
>> Noticed during code inspection.  The infinite loop was not observed.
>>
>> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>> v3:
>> const on next_entry() parameter
>> Remove spaces inside for loop braces
>> Remove inner if (!entry) check
>> Expand commit message to state noticed during code inspection
>>
>> v2:
>> continue (not break) for pin == -1.
>>
>> I added the next_entry() helper since putting the expression in the for
>> loop is a little cluttered.  The helper can also be re-used for other
>> instances within the file.
>> ---
>>   xen/arch/x86/io_apic.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index c384f10c1b..c35d611ecf 100644
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -1586,20 +1586,24 @@ 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;
>> +}
> 
> When replying to the v2 thread I hadn't spotted yet that a v3 was already
> posted. As indicated, imo this name to too generic (now). I'd be happy to
> make adjustments while committing, as long as we can agree on some less
> generic name.

pin_list_next() works for me.

Thanks,
Jason