ARM: perf: don't warn about missing interrupt-affinity property for PPIs

PATCH New Will Deacon 4 participants 13 messages Apr 23, 2015 – May 1, 2015

Subsystems (4)

Arm (32-bit) perf events arm64 Arm PMU profiling and debugging

lore

Share & tools

lore

Will Deacon

author

2015-04-23 13:50 UTC #0 lore raw

PATCH 1/4 ARM: perf: don't warn about missing interrupt-affinity property for PPIs

PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.

Reported-by Maxime Ripard <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/kernel/perf_event_cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

arch/arm/kernel/perf_event_cpu.c +6 -1

index 91c7ba182dcd..becf7ad6eddc 100644
@@ -303,12 +303,17 @@ static int probe_current_pmu(struct arm_pmu *pmu)
 
 static int of_pmu_irq_cfg(struct platform_device *pdev)
 {
-	int i;
+	int i, irq;
 	int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
 
 	if (!irqs)
 		return -ENOMEM;
 
+	/* Don't bother with PPIs; they're already affine */
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0 && irq_is_percpu(irq))
+		return 0;
+
 	for (i = 0; i < pdev->num_resources; ++i) {
 		struct device_node *dn;
 		int cpu;

Signature · 1 line

2.1.4

Will Deacon

author

2015-04-23 13:50 UTC #1 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:

hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]

This patch fixes the name to that of the pmu node.

Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/kernel/perf_event_cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/kernel/perf_event_cpu.c +1 -1

index becf7ad6eddc..213919ba326f 100644
@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
 				      i);
 		if (!dn) {
 			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
 			break;
 		}

Signature · 1 line

2.1.4

Mark Rutland

maintainer

2015-04-23 13:58 UTC #2 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

Quoted 6 lines · Will Deacon

With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:

Quoted 6 lines

hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]
This patch fixes the name to that of the pmu node.
Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)
Signed-off-by Will Deacon <[email protected]>

Acked-by Mark Rutland <[email protected]>

Quoted 20 lines

---
 arch/arm/kernel/perf_event_cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/kernel/perf_event_cpu.c +1 -1

index becf7ad6eddc..213919ba326f 100644
@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
 				      i);
 		if (!dn) {
 			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
 			break;
 		}

Signature · 1 line

2.1.4

Sudeep Holla

2015-05-01 13:59 UTC #3 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

In reply to Will Deacon

Quoted 21 lines · Will Deacon

With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:
hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]
This patch fixes the name to that of the pmu node.
Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm/kernel/perf_event_cpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/kernel/perf_event_cpu.c +0 -0
index becf7ad6eddc..213919ba326f 100644

arch/arm/kernel/perf_event_cpu.c static int of_pmu_irq_cfg(struct platform_device *pdev) · line 326

Quoted 6 lines

arch/arm/kernel/perf_event_cpu.c +1 -1

@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
  				      i);
  		if (!dn) {
  			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
  			break;

With old DT we will see this message and one might think perf is broken. But since the code still assumes SPIs are listed in order of *logical* CPU number and continues to work, does it make sense to update the warning accordingly ?

Regards,
Sudeep

Will Deacon

author

2015-05-01 14:07 UTC #4 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

Quoted 17 lines · Sudeep Holla

Quoted 16 lines · Will Deacon
With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:
hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]
This patch fixes the name to that of the pmu node.
Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm/kernel/perf_event_cpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

arch/arm/kernel/perf_event_cpu.c static int of_pmu_irq_cfg(struct platform_device *pdev) · line 326
Quoted 11 lines
arch/arm/kernel/perf_event_cpu.c +1 -1
index becf7ad6eddc..213919ba326f 100644
@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
  				      i);
  		if (!dn) {
  			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
  			break;
With old DT we will see this message and one might think perf is broken. But since the code still assumes SPIs are listed in order of *logical* CPU number and continues to work, does it make sense to update the warning accordingly ?

Dunno; it doesn't work on Juno, for example. I've also already sent the arm64 pull, so it would need to be a separate patch if you wanted to print something different and we'd need a way to detect the cases where the routing is actually wrong.

Will

Mark Rutland

maintainer

2015-05-01 14:07 UTC #5 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

In reply to Sudeep Holla

Quoted 19 lines · Sudeep Holla

Quoted 16 lines · Will Deacon
With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:
hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]
This patch fixes the name to that of the pmu node.
Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm/kernel/perf_event_cpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

arch/arm/kernel/perf_event_cpu.c static int of_pmu_irq_cfg(struct platform_device *pdev) · line 326
Quoted 11 lines
arch/arm/kernel/perf_event_cpu.c +1 -1
index becf7ad6eddc..213919ba326f 100644
@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
  				      i);
  		if (!dn) {
  			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
  			break;
With old DT we will see this message and one might think perf is broken. But since the code still assumes SPIs are listed in order of *logical* CPU number and continues to work, does it make sense to update the warning accordingly ?

The issue is that while it may work in that configuration, it can easily be made to not work. So even if things happen to align, we should warn when we don't have explicit information regarding the affinity.

Do you have a suggestion for a better error message to cover that?

Thanks
Mark.

Sudeep Holla

2015-05-01 14:20 UTC #6 lore raw

PATCH 2/4 ARM perf: Fix the pmu node name in warning message

Quoted 37 lines · Mark Rutland

Quoted 19 lines · Sudeep Holla
Quoted 16 lines · Will Deacon
With commit 9fd85eb502a7 ("ARM: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:
hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]
This patch fixes the name to that of the pmu node.
Fixes 9fd85eb502a7 (ARM: pmu: add support for interrupt-affinity property)
Signed-off-by Will Deacon <[email protected]>
---
   arch/arm/kernel/perf_event_cpu.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
arch/arm/kernel/perf_event_cpu.c static int of_pmu_irq_cfg(struct platform_device *pdev) · line 326
Quoted 11 lines
arch/arm/kernel/perf_event_cpu.c +1 -1
index becf7ad6eddc..213919ba326f 100644
@@ -322,7 +322,7 @@ static int of_pmu_irq_cfg(struct platform_device *pdev)
   				      i);
   		if (!dn) {
   			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
   			break;
With old DT we will see this message and one might think perf is broken. But since the code still assumes SPIs are listed in order of *logical* CPU number and continues to work, does it make sense to update the warning accordingly ?

Quoted 6 lines

The issue is that while it may work in that configuration, it can easily be made to not work. So even if things happen to align, we should warn when we don't have explicit information regarding the affinity.
Do you have a suggestion for a better error message to cover that?

Yes I thought it could be reworded but since that might make people to ignore the warning and fail to fix DT. Also as Will suggested in the other mail we need a way to detect correctness of routing to give more appropriate message. So it's better to leave it as is.

Regards,
Sudeep

Will Deacon

author

2015-04-23 13:50 UTC #7 lore raw

PATCH 3/4 arm64: perf: don't warn about missing interrupt-affinity property for PPIs

In reply to Will Deacon

PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.

Reported-by Maxime Ripard <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/perf_event.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

arch/arm64/kernel/perf_event.c +6 -1

index 195991dadc37..2a9cbcb61126 100644
@@ -1310,7 +1310,7 @@ static const struct of_device_id armpmu_of_device_ids[] = {
 
 static int armpmu_device_probe(struct platform_device *pdev)
 {
-	int i, *irqs;
+	int i, irq, *irqs;
 
 	if (!cpu_pmu)
 		return -ENODEV;

@@ -1319,6 +1319,11 @@ static int armpmu_device_probe(struct platform_device *pdev)
 	if (!irqs)
 		return -ENOMEM;
 
+	/* Don't bother with PPIs; they're already affine */
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0 && irq_is_percpu(irq))
+		return 0;
+
 	for (i = 0; i < pdev->num_resources; ++i) {
 		struct device_node *dn;
 		int cpu;

Signature · 1 line

2.1.4

Will Deacon

author

2015-04-23 13:50 UTC #8 lore raw

PATCH 4/4 arm64: perf: Fix the pmu node name in warning message

In reply to Will Deacon

From: "Suzuki K. Poulose" <[email protected]>

With commit d5efd9cc9cf2 ("arm64: pmu: add support for interrupt-affinity property"), we print a warning when we find a PMU SPI with a missing missing interrupt-affinity property in a pmu node. Unfortunately, we pass the wrong (NULL) device node to of_node_full_name, resulting in unhelpful messages such as:

hw perfevents: Failed to parse <no-node>/interrupt-affinity[0]

This patch fixes the name to that of the pmu node.

Fixes d5efd9cc9cf2 (arm64: pmu: add support for interrupt-affinity property)

Acked-by Mark Rutland <[email protected]>

Signed-off-by Suzuki K. Poulose <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/perf_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/perf_event.c +1 -1

index 2a9cbcb61126..23f25acf43a9 100644
@@ -1332,7 +1332,7 @@ static int armpmu_device_probe(struct platform_device *pdev)
 				      i);
 		if (!dn) {
 			pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-				of_node_full_name(dn), i);
+				of_node_full_name(pdev->dev.of_node), i);
 			break;
 		}

Signature · 1 line

2.1.4

Mark Rutland

maintainer

2015-04-23 14:01 UTC #9 lore raw

PATCH 1/4 ARM: perf: don't warn about missing interrupt-affinity property for PPIs

In reply to Will Deacon

Quoted 18 lines · Will Deacon

PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.
Reported-by Maxime Ripard <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/kernel/perf_event_cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm/kernel/perf_event_cpu.c +0 -1
index 91c7ba182dcd..becf7ad6eddc 100644
@@ -303,12 +303,17 @@ static int probe_current_pmu(struct arm_pmu *pmu)
 
 static int of_pmu_irq_cfg(struct platform_device *pdev)
 {
-	int i;

arch/arm/kernel/perf_event_cpu.c static int probe_current_pmu(struct arm_pmu *pmu) · line 315

Quoted 10 lines

arch/arm/kernel/perf_event_cpu.c +5 -0

+	int i, irq;
 	int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
 
 	if (!irqs)
 		return -ENOMEM;
 
+	/* Don't bother with PPIs; they're already affine */
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0 && irq_is_percpu(irq))
+		return 0;

Is 0 still a valid IRQ on any ARM platforms?

Otherwise, looks fine to me.

Acked-by Mark Rutland <[email protected]>

Mark

Will Deacon

author

2015-04-23 14:02 UTC #10 lore raw

PATCH 1/4 ARM: perf: don't warn about missing interrupt-affinity property for PPIs

Quoted 25 lines · Mark Rutland

Quoted 24 lines · Will Deacon
PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.
Reported-by Maxime Ripard <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/kernel/perf_event_cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm/kernel/perf_event_cpu.c +1 -1
index 91c7ba182dcd..becf7ad6eddc 100644
@@ -303,12 +303,17 @@ static int probe_current_pmu(struct arm_pmu *pmu)
 
 static int of_pmu_irq_cfg(struct platform_device *pdev)
 {
-	int i;
+	int i, irq;
 	int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
 
 	if (!irqs)
 		return -ENOMEM;
 

Quoted 6 lines

arch/arm/kernel/perf_event_cpu.c static int probe_current_pmu(struct arm_pmu *pmu) · line 315
Quoted 4 lines
arch/arm/kernel/perf_event_cpu.c +4 -0
+	/* Don't bother with PPIs; they're already affine */
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0 && irq_is_percpu(irq))
+		return 0;
Is 0 still a valid IRQ on any ARM platforms?

I just went for consistency with the cpu_pmu_{request,free}_irq paths.

Will

Mark Rutland

maintainer

2015-04-23 14:11 UTC #11 lore raw

PATCH 1/4 ARM: perf: don't warn about missing interrupt-affinity property for PPIs

Quoted 26 lines · Will Deacon

Quoted 25 lines · Mark Rutland
Quoted 24 lines · Will Deacon
PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.
Reported-by Maxime Ripard <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/kernel/perf_event_cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm/kernel/perf_event_cpu.c +1 -1
index 91c7ba182dcd..becf7ad6eddc 100644
@@ -303,12 +303,17 @@ static int probe_current_pmu(struct arm_pmu *pmu)
 
 static int of_pmu_irq_cfg(struct platform_device *pdev)
 {
-	int i;
+	int i, irq;
 	int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
 
 	if (!irqs)
 		return -ENOMEM;
 

Quoted 8 lines

Quoted 6 lines
arch/arm/kernel/perf_event_cpu.c static int probe_current_pmu(struct arm_pmu *pmu) · line 315
Quoted 4 lines
arch/arm/kernel/perf_event_cpu.c +4 -0
+	/* Don't bother with PPIs; they're already affine */
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0 && irq_is_percpu(irq))
+		return 0;
Is 0 still a valid IRQ on any ARM platforms?
I just went for consistency with the cpu_pmu_{request,free}_irq paths.

Ah, ok. That makes sense.

I guess that also applies to the arm64 part, so feel free to add my ack to both.

Mark.

Maxime Ripard

2015-04-23 15:56 UTC #12 lore raw

PATCH 1/4 ARM: perf: don't warn about missing interrupt-affinity property for PPIs

In reply to Will Deacon

Quoted 5 lines · Will Deacon

PPIs are affine by nature, so the interrupt-affinity property is not used and therefore we shouldn't print a warning in its absence.
Reported-by Maxime Ripard <[email protected]>
Signed-off-by Will Deacon <[email protected]>

For this one and the arm64 one,

Reviewed-by Maxime Ripard <[email protected]>

Thanks!
Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150423/2a8fb712/attachment.sig>