Re: [PATCH v6 04/10] genirq: Introduce common irq_force_complete_move() implementation

Re: [PATCH v6 04/10] genirq: Introduce common irq_force_complete_move() implementation

[Frank Scheiner]

Hi there,

this change, specfically the introduction of irq_force_complete_move()
to `kernel/irq/migration.c`, strangely breaks our builds for the hp-sim
platform (i.e. Linux/ia64 for Ski):

```
  CC      kernel/irq/affinity.o
kernel/irq/migration.c: In function 'irq_force_complete_move':
kernel/irq/migration.c:40:72: error: 'struct irq_data' has no member named 'parent_data'
   40 |         for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = d->parent_data) {
      |                                                                        ^~
make[4]: *** [scripts/Makefile.build:207: kernel/irq/migration.o] Error 1
```

The reason seems to be that "d->parent_data" (i.e.
"irq_data.parent_data") is used unguarded in this function:

```
void irq_force_complete_move(struct irq_desc *desc)
{
    for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = d->parent_data) {
        if (d->chip && d->chip->irq_force_complete_move) {
            d->chip->irq_force_complete_move(d);
            return;
        }
    }
}
```

...but "parent_data" is only present in `include/linux/irq.h` if
`CONFIG_IRQ_DOMAIN_HIERARCHY` was selected.

```
struct irq_data {
    u32            mask;
    unsigned int        irq;
    irq_hw_number_t        hwirq;
    struct irq_common_data    *common;
    struct irq_chip        *chip;
    struct irq_domain    *domain;
#ifdef    CONFIG_IRQ_DOMAIN_HIERARCHY
    struct irq_data        *parent_data;
#endif
    void            *chip_data;
};
```

So I guess, either the requirement in `linux/include/linux/irq.h` needs
to go, or the use of "d->parent_data" or the whole of
irq_force_complete_move() and its use needs to be guarded as well.

Cheers,
Frank

Re: [PATCH v6 04/10] genirq: Introduce common irq_force_complete_move() implementation

[Thomas Gleixner]

On Thu, Apr 03 2025 at 15:13, Frank Scheiner wrote:
> this change, specfically the introduction of irq_force_complete_move()
> to `kernel/irq/migration.c`, strangely breaks our builds for the hp-sim
> platform (i.e. Linux/ia64 for Ski):

arch/itanic has hit the iceberg and sank with the release of v6.7...

> So I guess, either the requirement in `linux/include/linux/irq.h` needs
> to go, or the use of "d->parent_data" or the whole of
> irq_force_complete_move() and its use needs to be guarded as well.

Guessing is not a really good approach to solve a technical problem :)

Thanks,

        tglx
---
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -37,7 +37,7 @@ bool irq_fixup_move_pending(struct irq_d
 
 void irq_force_complete_move(struct irq_desc *desc)
 {
-	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = d->parent_data) {
+	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = irqd_get_parent_data(d)) {
 		if (d->chip && d->chip->irq_force_complete_move) {
 			d->chip->irq_force_complete_move(d);
 			return;

Re: [PATCH v6 04/10] genirq: Introduce common irq_force_complete_move() implementation

[Frank Scheiner]

On 03.04.25 17:03, Thomas Gleixner wrote:
> On Thu, Apr 03 2025 at 15:13, Frank Scheiner wrote:
>> this change, specfically the introduction of irq_force_complete_move()
>> to `kernel/irq/migration.c`, strangely breaks our builds for the hp-sim
>> platform (i.e. Linux/ia64 for Ski):
>
> arch/itanic has hit the iceberg and sank with the release of v6.7...

Ha, ha, not in our waters. :-) Still I'm glad I could help finding
errors in Linux by building for ia64.


>> So I guess, either the requirement in `linux/include/linux/irq.h` needs
>> to go, or the use of "d->parent_data" or the whole of
>> irq_force_complete_move() and its use needs to be guarded as well.
>
> Guessing is not a really good approach to solve a technical problem :)

Thanks for the advice, though guessing was not involved here. It's just
a way to say that I'm not sure what the right solution might be in this
case. And then I'd rather leave such fixes to the experts in the
specific field.

> ---
> --- a/kernel/irq/migration.c
> +++ b/kernel/irq/migration.c
> @@ -37,7 +37,7 @@ bool irq_fixup_move_pending(struct irq_d
>
>  void irq_force_complete_move(struct irq_desc *desc)
>  {
> -	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = d->parent_data) {
> +	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = irqd_get_parent_data(d)) {
>  		if (d->chip && d->chip->irq_force_complete_move) {
>  			d->chip->irq_force_complete_move(d);
>  			return;
>

This fxes the build for me for that file. Much obliged.
```
  CC      kernel/irq/migration.o
```

Cheers,
Frank

[PATCH] genirq/migration: Use irqd_get_parent_data() in irq_force_complete_move()

[Thomas Gleixner]

Frank reported, that the common irq_force_complete_move() breaks the out of
tree build of ia64. The reason is that ia64 uses the migration code, but
does not have hierarchical interrupt domains enabled.

This went unnoticed in mainline as both x86 and RISC-V have hierarchical
domains enabled. Not that it matters for mainline, but it's still
inconsistent.

Use irqd_get_parent_data() instead of accessing the parent_data field
directly. The helper returns NULL when hierarchical domains are disabled
otherwise it accesses the parent_data field of the domain.

No functional change.

Fixes: 751dc837dabd ("genirq: Introduce common irq_force_complete_move() implementation")
Reported-by: Frank Scheiner <frank.scheiner@web.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Frank Scheiner <frank.scheiner@web.de>

--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -37,7 +37,7 @@ bool irq_fixup_move_pending(struct irq_d
 
 void irq_force_complete_move(struct irq_desc *desc)
 {
-	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = d->parent_data) {
+	for (struct irq_data *d = irq_desc_get_irq_data(desc); d; d = irqd_get_parent_data(d)) {
 		if (d->chip && d->chip->irq_force_complete_move) {
 			d->chip->irq_force_complete_move(d);
 			return;