genirq: Sanitize irq_set_affinity callbacks

Review

Quoted 2 lines · Thomas Gleixner

(quoted diff) +2 -0

+	i = cpumask_first_and(mask< cpu_online_mask);
+	i = cpumask_first_and(mask< cpu_online_mask);

s/</,/ ?

The user space interface does not filter out offline cpus. It merily verifies that the mask contains at least one online cpu. So the selector in the irq chip implementation needs to make sure to pick only an online cpu because otherwise:

Offline Core 1
Set affinity to 0xe
Selector will pick first set bit, i.e. core 1

The user space interface does not filter out offline cpus. It merily verifies that the mask contains at least one online cpu. So the selector in the irq chip implementation needs to make sure to pick only an online cpu because otherwise:

Offline Core 1
Set affinity to 0xe
Selector will pick first set bit, i.e. core 1

The set_irq_affinity() function has two issues:

1) It has no protection against selecting an offline cpu from the given mask.

2) It pointlessly restricts the affinity masks to have a single cpu set. This collides with the irq migration code of arm.

irq affinity is set to core 3
core 3 goes offline

migration code sets mask to cpu_online_mask and calls the irq_set_affinity() callback of the irq_chip which fails due to bit 0,1,2 set.

So instead of doing silly for_each_cpu() loops just pick any bit of the mask which intersects with the online mask.

The read back of the routing register is pointless as well. We can simply write the new mask as the bits in this register reflect one core.

Get rid of fiddling with the default_irq_affinity as well.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Jason Cooper <[email protected]>

---
 drivers/irqchip/irq-armada-370-xp.c |   38 ++++--------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

Index: tip/drivers/irqchip/irq-armada-370-xp.c
===================================================================

drivers/irqchip/irq-armada-370-xp.c +5 -34

@@ -244,35 +244,16 @@ static DEFINE_RAW_SPINLOCK(irq_controlle
 static int armada_xp_set_affinity(struct irq_data *d,
 				  const struct cpumask *mask_val, bool force)
 {
-	unsigned long reg;
-	unsigned long new_mask = 0;
-	unsigned long online_mask = 0;
-	unsigned long count = 0;
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long mask;
 	int cpu;
 
-	for_each_cpu(cpu, mask_val) {
-		new_mask |= 1 << cpu_logical_map(cpu);
-		count++;
-	}
-
-	/*
-	 * Forbid mutlicore interrupt affinity
-	 * This is required since the MPIC HW doesn't limit
-	 * several CPUs from acknowledging the same interrupt.
-	 */
-	if (count > 1)
-		return -EINVAL;
-
-	for_each_cpu(cpu, cpu_online_mask)
-		online_mask |= 1 << cpu_logical_map(cpu);
+	/* Select a single core from the affinity mask which is online */
+	cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	mask = 1UL << cpu_logical_map(cpu);
 
 	raw_spin_lock(&irq_controller_lock);
-
-	reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
-	reg = (reg & (~online_mask)) | new_mask;
-	writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
-
+	writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
 	raw_spin_unlock(&irq_controller_lock);
 
 	return 0;

@@ -494,15 +475,6 @@ static int __init armada_370_xp_mpic_of_
 
 #ifdef CONFIG_SMP
 	armada_xp_mpic_smp_cpu_init();
-
-	/*
-	 * Set the default affinity from all CPUs to the boot cpu.
-	 * This is required since the MPIC doesn't limit several CPUs
-	 * from acknowledging the same interrupt.
-	 */
-	cpumask_clear(irq_default_affinity);
-	cpumask_set_cpu(smp_processor_id(), irq_default_affinity);
-
 #endif
 
 	armada_370_xp_msi_init(node, main_int_res.start);
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas,

nit: s/armanda/armada/ in the patch subject.

Gregory,

Mind providing an Ack on this?

thx,

Jason.

Quoted 100 lines · Thomas Gleixner

The set_irq_affinity() function has two issues:

1) It has no protection against selecting an offline cpu from the given mask.

2) It pointlessly restricts the affinity masks to have a single cpu set. This collides with the irq migration code of arm.

irq affinity is set to core 3
core 3 goes offline

migration code sets mask to cpu_online_mask and calls the irq_set_affinity() callback of the irq_chip which fails due to bit 0,1,2 set.

So instead of doing silly for_each_cpu() loops just pick any bit of the mask which intersects with the online mask.

The read back of the routing register is pointless as well. We can simply write the new mask as the bits in this register reflect one core.

Get rid of fiddling with the default_irq_affinity as well.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Jason Cooper <[email protected]>

---
 drivers/irqchip/irq-armada-370-xp.c |   38 ++++--------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

Index: tip/drivers/irqchip/irq-armada-370-xp.c
===================================================================

drivers/irqchip/irq-armada-370-xp.c +5 -34

@@ -244,35 +244,16 @@ static DEFINE_RAW_SPINLOCK(irq_controlle
 static int armada_xp_set_affinity(struct irq_data *d,
 				  const struct cpumask *mask_val, bool force)
 {
-	unsigned long reg;
-	unsigned long new_mask = 0;
-	unsigned long online_mask = 0;
-	unsigned long count = 0;
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long mask;
 	int cpu;
 
-	for_each_cpu(cpu, mask_val) {
-		new_mask |= 1 << cpu_logical_map(cpu);
-		count++;
-	}
-
-	/*
-	 * Forbid mutlicore interrupt affinity
-	 * This is required since the MPIC HW doesn't limit
-	 * several CPUs from acknowledging the same interrupt.
-	 */
-	if (count > 1)
-		return -EINVAL;
-
-	for_each_cpu(cpu, cpu_online_mask)
-		online_mask |= 1 << cpu_logical_map(cpu);
+	/* Select a single core from the affinity mask which is online */
+	cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	mask = 1UL << cpu_logical_map(cpu);
 
 	raw_spin_lock(&irq_controller_lock);
-
-	reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
-	reg = (reg & (~online_mask)) | new_mask;
-	writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
-
+	writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
 	raw_spin_unlock(&irq_controller_lock);
 
 	return 0;

@@ -494,15 +475,6 @@ static int __init armada_370_xp_mpic_of_
 
 #ifdef CONFIG_SMP
 	armada_xp_mpic_smp_cpu_init();
-
-	/*
-	 * Set the default affinity from all CPUs to the boot cpu.
-	 * This is required since the MPIC doesn't limit several CPUs
-	 * from acknowledging the same interrupt.
-	 */
-	cpumask_clear(irq_default_affinity);
-	cpumask_set_cpu(smp_processor_id(), irq_default_affinity);
-
 #endif
 
 	armada_370_xp_msi_init(node, main_int_res.start);
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 4 lines · Jason Cooper

Thomas,

nit: s/armanda/armada/ in the patch subject.

Quoted 3 lines

Gregory,

Mind providing an Ack on this?

Well sorry but with this patch the kernel doesn't
work anymore.

I am investigating to find if some part could be kept.

Quoted 9 lines · Gregory CLEMENT

Quoted 7 lines · Jason Cooper

Thomas,

nit: s/armanda/armada/ in the patch subject.

Gregory,

Mind providing an Ack on this?

Quoted 4 lines

Well sorry but with this patch the kernel doesn't
work anymore.

I am investigating to find if some part could be kept.

It might be the readback of the routing register. I don't have the datasheet of this.

Thanks,

tglx

Quoted 2 lines · Thomas Gleixner

It might be the readback of the routing register. I don't have the datasheet of this.

Sorry for the delay, I was on vacation without the hardware to test it.

Indeed it was the readback of the routing register. (Unfortunately the datasheet was not yet publicly available :( ). In your patch by replacing the line:

writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

by the following ones:

reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq)); reg = (reg & (~ARMADA_370_XP_INT_SOURCE_CPU_MASK)) | mask; writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

with
#define ARMADA_370_XP_INT_SOURCE_CPU_MASK 0xF

Then it worked.

About masking with the online CPU in the original code, the purpose was to allow sharing the SoC with an other OS by doing AMP. This feature is part of the Marvell version of the kernel.
The idea was to bring this latter in the vanilla kernel, but I am not sure that all the part needed for AMP are acceptable for mainline. So I can add it back later when we will need it.

Quoted 21 lines · Gregory CLEMENT

Quoted 2 lines · Thomas Gleixner

It might be the readback of the routing register. I don't have the datasheet of this.

Sorry for the delay, I was on vacation without the hardware to test it.

Indeed it was the readback of the routing register. (Unfortunately the datasheet was not yet publicly available :( ). In your patch by replacing the line:

writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

by the following ones:

reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq)); reg = (reg & (~ARMADA_370_XP_INT_SOURCE_CPU_MASK)) | mask; writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

with
#define ARMADA_370_XP_INT_SOURCE_CPU_MASK 0xF

Quoted 8 lines

Then it worked.

About masking with the online CPU in the original code, the purpose was to allow sharing the SoC with an other OS by doing AMP. This feature is part of the Marvell version of the kernel.
The idea was to bring this latter in the vanilla kernel, but I am not sure that all the part needed for AMP are acceptable for mainline. So I can add it back later when we will need it.

Right.

So are you ok with that patch (including your change) ? If you send me tested/acked-by i'll route it for 3.15

Thanks,

tglx

Quoted 33 lines · Thomas Gleixner

Quoted 21 lines · Gregory CLEMENT

Quoted 2 lines · Thomas Gleixner

It might be the readback of the routing register. I don't have the datasheet of this.

Sorry for the delay, I was on vacation without the hardware to test it.

Indeed it was the readback of the routing register. (Unfortunately the datasheet was not yet publicly available :( ). In your patch by replacing the line:

writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

by the following ones:

reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq)); reg = (reg & (~ARMADA_370_XP_INT_SOURCE_CPU_MASK)) | mask; writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

with
#define ARMADA_370_XP_INT_SOURCE_CPU_MASK 0xF

Quoted 8 lines

Then it worked.

About masking with the online CPU in the original code, the purpose was to allow sharing the SoC with an other OS by doing AMP. This feature is part of the Marvell version of the kernel.
The idea was to bring this latter in the vanilla kernel, but I am not sure that all the part needed for AMP are acceptable for mainline. So I can add it back later when we will need it.

Right.

Quoted 2 lines

So are you ok with that patch (including your change) ? If you send me tested/acked-by i'll route it for 3.15

With the change I included you can add my

Acked-by Gregory CLEMENT <[email protected]>

Tested-by Gregory CLEMENT <[email protected]>

Hi Thomas,

Quoted 39 lines · Gregory CLEMENT

Quoted 31 lines · Thomas Gleixner

Quoted 21 lines · Gregory CLEMENT

Quoted 2 lines · Thomas Gleixner

It might be the readback of the routing register. I don't have the datasheet of this.

Sorry for the delay, I was on vacation without the hardware to test it.

Indeed it was the readback of the routing register. (Unfortunately the datasheet was not yet publicly available :( ). In your patch by replacing the line:

writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

by the following ones:

reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq)); reg = (reg & (~ARMADA_370_XP_INT_SOURCE_CPU_MASK)) | mask; writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));

with
#define ARMADA_370_XP_INT_SOURCE_CPU_MASK 0xF

Quoted 8 lines

Then it worked.

About masking with the online CPU in the original code, the purpose was to allow sharing the SoC with an other OS by doing AMP. This feature is part of the Marvell version of the kernel.
The idea was to bring this latter in the vanilla kernel, but I am not sure that all the part needed for AMP are acceptable for mainline. So I can add it back later when we will need it.

Quoted 4 lines

Right.

So are you ok with that patch (including your change) ? If you send me tested/acked-by i'll route it for 3.15

With the change I included you can add my

Quoted 3 lines

Acked-by Gregory CLEMENT <[email protected]>

Tested-by Gregory CLEMENT <[email protected]>

I have just noticed that this patch was not merged in 3.15. Actually it is not in the core/irq branch of tip.git with the other patches of the same series.
Is there any reason for this?

Quoted 5 lines · Gregory CLEMENT

I have just noticed that this patch was not merged in 3.15. Actually it is not in the core/irq branch of tip.git with the other patches of the same series.
Is there any reason for this?

I know your mail filters are pretty aggressive and I just wanted to make sure you saw this. I'm also not sure what happened here.