drivers/perf: arm_pmu: fix legacy affinity-less DT support

PATCH New Mark Rutland 2 participants 4 messages Aug 9, 2016 Arm PMU profiling and debugging

Share & tools

Mark Rutland

author maintainer

2016-08-09 15:51 UTC #0 lore raw

PATCH 1/1 drivers/perf: arm_pmu: fix legacy affinity-less DT support

Commit 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask") relies on using_spi being valid, but this is only initialised correctly in the presence of an interrupt-affinity property, which legacy DTBs do not have.

In the absence of an interrupt-affinity property, using_spi is always false (regardless of whether SPIs are actually used), so we call irq_get_percpu_devid_partition(irq). This returns -EINVAL, and we give up, passing on this return value.

The code which determines using_spi also verifies that we do not have mixed SPI/PPI interrupts. Even in the absence of an interrupt-affinity property we do not support mixed SPI/PPI cases, so pull the validation logic above the main loop, ensuring that using_spi is always valid.

At the same time, have the error message to give the path of the PMU node, rather than a CPU node, as the mismatch is a property of the entire set of PMU interrupts rather than a particular CPU associated with it.

Fixes 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask")

Signed-off-by Mark Rutland <[email protected]>

Reported-by Geert Uytterhoeven <[email protected]>

Reported-by Robin Murphy <[email protected]>

Tested-by Robin Murphy <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

It's been pointed out to me that Marc send a patch a while ago [1] fixing the same issue, but for some reason that didn't get queued.

This patch had the added benefit of ensuring we always avoid mismatched SPI/PPI mixes, but otherwise either patch should avoid the issue.

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/444404.html

drivers/perf/arm_pmu.c +20 ~~-17~~

index 6ccb994..7d3c690 100644
@@ -900,38 +900,41 @@ static int probe_current_pmu(struct arm_pmu *pmu,
 
 static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 {
-	int *irqs, i = 0;
+	int *irqs, i;
 	bool using_spi = false;
 	struct platform_device *pdev = pmu->plat_device;
 
+	/* Check the IRQ type and prohibit a mix of PPIs and SPIs */
+	for (i = 0; i < pdev->num_resources; i++) {
+		int irq = platform_get_irq(pdev, i);
+		if (irq >= 0) {
+			bool spi = !irq_is_percpu(irq);
+
+			if (i > 0 && spi != using_spi) {
+				pr_err("PPI/SPI IRQ type mismatch for %s!\n",
+					of_node_full_name(pdev->dev.of_node));
+				return -EINVAL;
+			}
+
+			using_spi = spi;
+		}
+	}
+
 	irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
 	if (!irqs)
 		return -ENOMEM;
 
+	i = 0;
+
 	do {
 		struct device_node *dn;
-		int cpu, irq;
+		int cpu;
 
 		/* See if we have an affinity entry */
 		dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity", i);
 		if (!dn)
 			break;
 
-		/* Check the IRQ type and prohibit a mix of PPIs and SPIs */
-		irq = platform_get_irq(pdev, i);
-		if (irq >= 0) {
-			bool spi = !irq_is_percpu(irq);
-
-			if (i > 0 && spi != using_spi) {
-				pr_err("PPI/SPI IRQ type mismatch for %s!\n",
-					dn->name);
-				kfree(irqs);
-				return -EINVAL;
-			}
-
-			using_spi = spi;
-		}
-
 		/* Now look up the logical CPU number */
 		for_each_possible_cpu(cpu) {
 			struct device_node *cpu_dn;

Signature · 1 line

1.9.1

Will Deacon

2016-08-09 16:48 UTC #1 lore raw

PATCH 1/1 drivers/perf: arm_pmu: fix legacy affinity-less DT support

Quoted 32 lines · Mark Rutland

Commit 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask") relies on using_spi being valid, but this is only initialised correctly in the presence of an interrupt-affinity property, which legacy DTBs do not have.
In the absence of an interrupt-affinity property, using_spi is always false (regardless of whether SPIs are actually used), so we call irq_get_percpu_devid_partition(irq). This returns -EINVAL, and we give up, passing on this return value.
The code which determines using_spi also verifies that we do not have mixed SPI/PPI interrupts. Even in the absence of an interrupt-affinity property we do not support mixed SPI/PPI cases, so pull the validation logic above the main loop, ensuring that using_spi is always valid.
At the same time, have the error message to give the path of the PMU node, rather than a CPU node, as the mismatch is a property of the entire set of PMU interrupts rather than a particular CPU associated with it.
Fixes 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask")
Signed-off-by Mark Rutland <[email protected]>
Reported-by Geert Uytterhoeven <[email protected]>
Reported-by Robin Murphy <[email protected]>
Tested-by Robin Murphy <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Will Deacon <[email protected]>
---
 drivers/perf/arm_pmu.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Quoted 5 lines

It's been pointed out to me that Marc send a patch a while ago [1] fixing the same issue, but for some reason that didn't get queued.
This patch had the added benefit of ensuring we always avoid mismatched SPI/PPI mixes, but otherwise either patch should avoid the issue.

I'm a little uneasy with that change, since we now go ahead and check all of the IRQ resources, regardless of the interrupt-affinity property. This could result in us failing the probe, where we could previously have done the right thing.

For now, I think I'll stick with Marc's patch (that somehow slipped through the woodwork), but maybe we should consider removing the SPI/PPI sanity checking altogether. At some point we have to rely on the DT being correct, and I don't think we do anything disastrout if there is a mismatch in interrupt types.

Will

Mark Rutland

author maintainer

2016-08-09 16:51 UTC #2 lore raw

PATCH 1/1 drivers/perf: arm_pmu: fix legacy affinity-less DT support

Quoted 44 lines · Will Deacon

Quoted 32 lines · Mark Rutland
Commit 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask") relies on using_spi being valid, but this is only initialised correctly in the presence of an interrupt-affinity property, which legacy DTBs do not have.
In the absence of an interrupt-affinity property, using_spi is always false (regardless of whether SPIs are actually used), so we call irq_get_percpu_devid_partition(irq). This returns -EINVAL, and we give up, passing on this return value.
The code which determines using_spi also verifies that we do not have mixed SPI/PPI interrupts. Even in the absence of an interrupt-affinity property we do not support mixed SPI/PPI cases, so pull the validation logic above the main loop, ensuring that using_spi is always valid.
At the same time, have the error message to give the path of the PMU node, rather than a CPU node, as the mismatch is a property of the entire set of PMU interrupts rather than a particular CPU associated with it.
Fixes 19a469a58720ea96 ("drivers/perf: arm-pmu: Handle per-interrupt affinity mask")
Signed-off-by Mark Rutland <[email protected]>
Reported-by Geert Uytterhoeven <[email protected]>
Reported-by Robin Murphy <[email protected]>
Tested-by Robin Murphy <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Will Deacon <[email protected]>
---
 drivers/perf/arm_pmu.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)
Quoted 5 lines
It's been pointed out to me that Marc send a patch a while ago [1] fixing the same issue, but for some reason that didn't get queued.
This patch had the added benefit of ensuring we always avoid mismatched SPI/PPI mixes, but otherwise either patch should avoid the issue.
I'm a little uneasy with that change, since we now go ahead and check all of the IRQ resources, regardless of the interrupt-affinity property. This could result in us failing the probe, where we could previously have done the right thing.

Quoted 5 lines

For now, I think I'll stick with Marc's patch (that somehow slipped through the woodwork), but maybe we should consider removing the SPI/PPI sanity checking altogether. At some point we have to rely on the DT being correct, and I don't think we do anything disastrout if there is a mismatch in interrupt types.

Sure thing. From a quick scan it looks like we'd safely bail out at the point we try to request the IRQ (i.e. when we set up the first event).

FWIW, for Marc's patch, assuming you haven't queued it yet:

Acked-by Mark Rutland <[email protected]>

Thanks,
Mark.

Will Deacon

2016-08-09 16:52 UTC #3 lore raw

PATCH 1/1 drivers/perf: arm_pmu: fix legacy affinity-less DT support

In reply to Will Deacon

Quoted 5 lines · Will Deacon

On Tue, Aug 09, 2016 at 04:51:07PM +0100, Mark Rutland wrote:
For now, I think I'll stick with Marc's patch (that somehow slipped through the woodwork), but maybe we should consider removing the SPI/PPI sanity checking altogether. At some point we have to rely on the DT being correct, and I don't think we do anything disastrout if there is a mismatch in

                                        ^^^^^

Oh dear, mind on other things.

Will