Re: [patch V2 04/46] genirq/chip: Use the first chip in irq_chip_compose_msi_msg()

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Wed, Aug 26 2020 at 20:50, Marc Zyngier wrote:
> On Wed, 26 Aug 2020 12:16:32 +0100,
> Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> ---
>> V2: New patch. Note, that this might break other stuff which relies on the
>>     current behaviour, but the hierarchy composition of DT based chips is
>>     really hard to follow.
>
> Grepping around, I don't think there is any occurrence of two irqchips
> providing irq_compose_msi() that can share a hierarchy on any real
> system, so we should be fine. Famous last words.

Knocking on wood :)

>>  #ifdef      CONFIG_IRQ_DOMAIN_HIERARCHY
>> -    for (; data; data = data->parent_data)
>> -#endif
>> -            if (data->chip && data->chip->irq_compose_msi_msg)
>> +    for (; data; data = data->parent_data) {
>> +            if (data->chip && data->chip->irq_compose_msi_msg) {
>>                      pos = data;
>> +                    break;
>> +            }
>> +    }
>> +#else
>> +    if (data->chip && data->chip->irq_compose_msi_msg)
>> +            pos = data;
>> +#endif
>>      if (!pos)
>>              return -ENOSYS;
>
> Is it just me, or is this last change more complex than it ought to
> be?

Kinda.

> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 857f5f4c8098..25e18b73699c 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1544,7 +1544,7 @@ int irq_chip_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>       struct irq_data *pos = NULL;
>  
>  #ifdef       CONFIG_IRQ_DOMAIN_HIERARCHY
> -     for (; data; data = data->parent_data)
> +     for (; data && !pos; data = data->parent_data)
>  #endif
>               if (data->chip && data->chip->irq_compose_msi_msg)
>                       pos = data;
>
> Though the for loop in a #ifdef in admittedly an acquired taste...

Checking !pos is simpler obviously. That doesn't make me hate the loop
in the #ifdef less. :)

What about the below?

Thanks,

        tglx
---
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -473,6 +473,15 @@ static inline void irq_domain_deactivate
 }
 #endif
 
+static inline struct irq_data *irqd_get_parent_data(struct irq_data *irqd)
+{
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+       return irqd->parent_data;
+#else
+       return NULL;
+#endif
+}
+
 #ifdef CONFIG_GENERIC_IRQ_DEBUGFS
 #include <linux/debugfs.h>
 
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1541,18 +1541,17 @@ EXPORT_SYMBOL_GPL(irq_chip_release_resou
  */
 int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
-       struct irq_data *pos = NULL;
+       struct irq_data *pos;
 
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
-       for (; data; data = data->parent_data)
-#endif
+       for (pos = NULL; !pos && data; data = irqd_get_parent_data(data)) {
                if (data->chip && data->chip->irq_compose_msi_msg)
                        pos = data;
+       }
+
        if (!pos)
                return -ENOSYS;
 
        pos->chip->irq_compose_msi_msg(pos, msg);
-
        return 0;
 }