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

From: Thomas Gleixner
Date: Wed Aug 26 2020 - 17:20:05 EST


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;
}