The irqd_set_move_pending() and irq_copy_pending() appear in pairs, but
irq_copy_pending() is empty when CONFIG_GENERIC_PENDING_IRQ is not set,
irqd_set_move_pending always set IRQD_SETAFFINITY_PENDING flag.

And before commit 1fa46f1f0709 ("genirq: Simplify affinity related code"),
if the config not set, IRQ_MOVE_PENDING will not try set and
desc->pending_mask will not be copied no matter what. Fix it by combining
them to align with them, and define empty for both if the config
is not enabled.

Fixes: 1fa46f1f0709 ("genirq: Simplify affinity related code")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 kernel/irq/internals.h |  7 +++++--
 kernel/irq/manage.c    | 19 +++++--------------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index fe0272cd84a5..f3d83f3745cb 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -428,8 +428,11 @@ static inline bool irq_move_pending(struct irq_data *data)
 	return irqd_is_setaffinity_pending(data);
 }
 static inline void
-irq_copy_pending(struct irq_desc *desc, const struct cpumask *mask)
+irq_set_copy_pending(struct irq_data *data, const struct cpumask *mask)
 {
+	struct irq_desc *desc = irq_data_to_desc(data);
+
+	irqd_set_move_pending(data);
 	cpumask_copy(desc->pending_mask, mask);
 }
 static inline void
@@ -456,7 +459,7 @@ static inline bool irq_move_pending(struct irq_data *data)
 	return false;
 }
 static inline void
-irq_copy_pending(struct irq_desc *desc, const struct cpumask *mask)
+irq_set_copy_pending(struct irq_data *data, const struct cpumask *mask)
 {
 }
 static inline void
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f0803d6bd296..d03c3c4a869c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -293,23 +293,16 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	return ret;
 }
 
-#ifdef CONFIG_GENERIC_PENDING_IRQ
 static inline int irq_set_affinity_pending(struct irq_data *data,
 					   const struct cpumask *dest)
 {
-	struct irq_desc *desc = irq_data_to_desc(data);
-
-	irqd_set_move_pending(data);
-	irq_copy_pending(desc, dest);
+	irq_set_copy_pending(data, dest);
+#ifdef CONFIG_GENERIC_PENDING_IRQ
 	return 0;
-}
 #else
-static inline int irq_set_affinity_pending(struct irq_data *data,
-					   const struct cpumask *dest)
-{
 	return -EBUSY;
-}
 #endif
+}
 
 static int irq_try_set_affinity(struct irq_data *data,
 				const struct cpumask *dest, bool force)
@@ -365,10 +358,8 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 
 	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
 		ret = irq_try_set_affinity(data, mask, force);
-	} else {
-		irqd_set_move_pending(data);
-		irq_copy_pending(desc, mask);
-	}
+	} else
+		irq_set_copy_pending(data, mask);
 
 	if (desc->affinity_notify) {
 		kref_get(&desc->affinity_notify->kref);
-- 
2.34.1

On Fri, Aug 30 2024 at 18:09, Jinjie Ruan wrote:
> The irqd_set_move_pending() and irq_copy_pending() appear in pairs, but
> irq_copy_pending() is empty when CONFIG_GENERIC_PENDING_IRQ is not set,
> irqd_set_move_pending always set IRQD_SETAFFINITY_PENDING flag.

Which will never happen if CONFIG_GENERIC_PENDING_IRQ is not set.

> And before commit 1fa46f1f0709 ("genirq: Simplify affinity related code"),
> if the config not set, IRQ_MOVE_PENDING will not try set and

# git grep IRQ_MOVE_PENDING
# 

> desc->pending_mask will not be copied no matter what. Fix it by combining
> them to align with them, and define empty for both if the config
> is not enabled.
>
> Fixes: 1fa46f1f0709 ("genirq: Simplify affinity related code")

What does this actually fix?

You fail to explain what the actual consequence and failure is. If your
change is not fixing anything then there is no reason for a fixes tag.

>  static inline void
> -irq_copy_pending(struct irq_desc *desc, const struct cpumask *mask)
> +irq_set_copy_pending(struct irq_data *data, const struct cpumask *mask)
>  {
> +	struct irq_desc *desc = irq_data_to_desc(data);
> +
> +	irqd_set_move_pending(data);
>  	cpumask_copy(desc->pending_mask, mask);
>  }

How is that different from the existing irq_set_affinity_pending() ?

> -#ifdef CONFIG_GENERIC_PENDING_IRQ
>  static inline int irq_set_affinity_pending(struct irq_data *data,
>  					   const struct cpumask *dest)
>  {
> -	struct irq_desc *desc = irq_data_to_desc(data);
> -
> -	irqd_set_move_pending(data);
> -	irq_copy_pending(desc, dest);
> +	irq_set_copy_pending(data, dest);
> +#ifdef CONFIG_GENERIC_PENDING_IRQ

No. We don't put the ifdefs into the function. That's horrible to read.

Thanks,

        tglx