perf/arm_cspmu: Fixes and cleanups

PATCH Superseded Robin Murphy 4 participants 15 messages Jun 1, 2023 – Jun 5, 2023

Subsystems (3)

ACPI Arm PMU profiling and debugging ACPI arm64

lore

Share & tools

lore

Robin Murphy

author

2023-06-01 11:59 UTC #0 lore raw

cover perf/arm_cspmu: Fixes and cleanups

Hi all,

I've been working on adding support for the 64-bit extension and devicetree probing[1], which I'm not posting yet since it's still untested and may need a bit more work. However I think these preparatory patches stand up well enough on their own so could land sooner.

Thanks,
Robin.

[1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/cspmu-dev/

Robin Murphy (4):
perf/arm_cspmu: Fix event attribute type
ACPI/APMT: Don't register invalid resource
perf/arm_cspmu: Clean up ACPI dependency
perf/arm_cspmu: Decouple APMT dependency

 drivers/acpi/arm64/apmt.c          | 10 +++--
 drivers/perf/arm_cspmu/Kconfig     |  3 +-
 drivers/perf/arm_cspmu/arm_cspmu.c | 70 +++++++++++++-----------------
 drivers/perf/arm_cspmu/arm_cspmu.h |  4 +-
 4 files changed, 39 insertions(+), 48 deletions(-)

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Robin Murphy

author

2023-06-01 11:59 UTC #1 lore raw

PATCH 1/4 perf/arm_cspmu: Fix event attribute type

ARM_CSPMU_EVENT_ATTR() defines a struct perf_pmu_events_attr, so arm_cspmu_sysfs_event_show() should not be interpreting it as struct dev_ext_attribute.

Fixes e37dfd65731d ("perf: arm_cspmu: Add support for ARM CoreSight PMU driver")

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +4 -4

index a3f1c410b417..72dc7a9e1ca8 100644
@@ -189,10 +189,10 @@ static inline bool use_64b_counter_reg(const struct arm_cspmu *cspmu)
 ssize_t arm_cspmu_sysfs_event_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct dev_ext_attribute *eattr =
-		container_of(attr, struct dev_ext_attribute, attr);
-	return sysfs_emit(buf, "event=0x%llx\n",
-			  (unsigned long long)eattr->var);
+	struct perf_pmu_events_attr *pmu_attr;
+
+	pmu_attr = container_of(attr, typeof(*pmu_attr), attr);
+	return sysfs_emit(buf, "event=0x%llx\n", pmu_attr->id);
 }
 EXPORT_SYMBOL_GPL(arm_cspmu_sysfs_event_show);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Suzuki K Poulose

Reviewed-by on v1 1/4

2023-06-02 10:26 UTC #2 lore raw

Re: [PATCH 1/4] perf/arm_cspmu: Fix event attribute type

Quoted 17 lines · Robin Murphy

ARM_CSPMU_EVENT_ATTR() defines a struct perf_pmu_events_attr, so arm_cspmu_sysfs_event_show() should not be interpreting it as struct dev_ext_attribute.
Fixes e37dfd65731d ("perf: arm_cspmu: Add support for ARM CoreSight PMU driver")
Signed-off-by Robin Murphy <[email protected]>
---
  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
index a3f1c410b417..72dc7a9e1ca8 100644
@@ -189,10 +189,10 @@ static inline bool use_64b_counter_reg(const struct arm_cspmu *cspmu)
  ssize_t arm_cspmu_sysfs_event_show(struct device *dev,
  				struct device_attribute *attr, char *buf)

drivers/perf/arm_cspmu/arm_cspmu.c static inline bool use_64b_counter_reg(const struct arm_cspmu *cspmu) · line 196

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +4 -4

  {
-	struct dev_ext_attribute *eattr =
-		container_of(attr, struct dev_ext_attribute, attr);
-	return sysfs_emit(buf, "event=0x%llx\n",
-			  (unsigned long long)eattr->var);
+	struct perf_pmu_events_attr *pmu_attr;
+
+	pmu_attr = container_of(attr, typeof(*pmu_attr), attr);
+	return sysfs_emit(buf, "event=0x%llx\n", pmu_attr->id);
  }

Thanks for catching this ! The other option would be to, which was the original intention.

drivers/perf/arm_cspmu/arm_cspmu.h +0 -0

index 51323b175a4a..b5e454be95b3 100644
@@ -32,7 +32,7 @@
        ARM_CSPMU_EXT_ATTR(_name, arm_cspmu_sysfs_format_show, (char

*)_config)

#define ARM_CSPMU_EVENT_ATTR(_name, _config) \
- PMU_EVENT_ATTR_ID(_name, arm_cspmu_sysfs_event_show, _config)
+ ARM_CSPMU_EXT_ATTR(_name, arm_cspmu_sysfs_event_show, (void*)(unsigned long)_config)

/* Default event id mask */

Either way,

Reviewed-by Suzuki K Poulose <[email protected]>

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ilkka Koskinen

2023-06-05 07:30 UTC #3 lore raw

Re: [PATCH 1/4] perf/arm_cspmu: Fix event attribute type

In reply to Robin Murphy

Quoted 6 lines · Robin Murphy

ARM_CSPMU_EVENT_ATTR() defines a struct perf_pmu_events_attr, so arm_cspmu_sysfs_event_show() should not be interpreting it as struct dev_ext_attribute.
Fixes e37dfd65731d ("perf: arm_cspmu: Add support for ARM CoreSight PMU driver")
Signed-off-by Robin Murphy <[email protected]>

Reviewed-and-tested-by Ilkka Koskinen <[email protected]>

Quoted 27 lines

---
drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +4 -4

index a3f1c410b417..72dc7a9e1ca8 100644
@@ -189,10 +189,10 @@ static inline bool use_64b_counter_reg(const struct arm_cspmu *cspmu)
 ssize_t arm_cspmu_sysfs_event_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct dev_ext_attribute *eattr =
-		container_of(attr, struct dev_ext_attribute, attr);
-	return sysfs_emit(buf, "event=0x%llx\n",
-			  (unsigned long long)eattr->var);
+	struct perf_pmu_events_attr *pmu_attr;
+
+	pmu_attr = container_of(attr, typeof(*pmu_attr), attr);
+	return sysfs_emit(buf, "event=0x%llx\n", pmu_attr->id);
 }
 EXPORT_SYMBOL_GPL(arm_cspmu_sysfs_event_show);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Robin Murphy

author

2023-06-01 11:59 UTC #4 lore raw

PATCH 2/4 ACPI/APMT: Don't register invalid resource

In reply to Robin Murphy

Don't register a resource for the second page unless the dual-page extension flag is actually present to say it's valid.

CC Lorenzo Pieralisi <[email protected]>

CC Hanjun Guo <[email protected]>

CC Sudeep Holla <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

Not a critical fix, since the driver currently works around this itself, but patch #4 would like to clean that up.

 drivers/acpi/arm64/apmt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/arm64/apmt.c +6 -4

index 8cab69fa5d59..aa7d5c3c0dd8 100644
@@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
 
 	num_res++;
 
-	res[num_res].start = node->base_address1;
-	res[num_res].end = node->base_address1 + SZ_4K - 1;
-	res[num_res].flags = IORESOURCE_MEM;
+	if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
+		res[num_res].start = node->base_address1;
+		res[num_res].end = node->base_address1 + SZ_4K - 1;
+		res[num_res].flags = IORESOURCE_MEM;
 
-	num_res++;
+		num_res++;
+	}
 
 	if (node->ovflw_irq != 0) {
 		trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Suzuki K Poulose

Reviewed-by on v1 2/4

2023-06-02 10:27 UTC #5 lore raw

Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

Quoted 27 lines · Robin Murphy

Don't register a resource for the second page unless the dual-page extension flag is actually present to say it's valid.
CC Lorenzo Pieralisi <[email protected]>
CC Hanjun Guo <[email protected]>
CC Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
Not a critical fix, since the driver currently works around this itself, but patch #4 would like to clean that up.
  drivers/acpi/arm64/apmt.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/arm64/apmt.c +1 -3
index 8cab69fa5d59..aa7d5c3c0dd8 100644
@@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
 
  	num_res++;
 
-	res[num_res].start = node->base_address1;
-	res[num_res].end = node->base_address1 + SZ_4K - 1;
-	res[num_res].flags = IORESOURCE_MEM;
+	if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {

drivers/acpi/arm64/apmt.c static int __init apmt_init_resources(struct resource *res, · line 47

Quoted 10 lines

drivers/acpi/arm64/apmt.c +5 -1

+		res[num_res].start = node->base_address1;
+		res[num_res].end = node->base_address1 + SZ_4K - 1;
+		res[num_res].flags = IORESOURCE_MEM;
 
-	num_res++;
+		num_res++;
+	}
 
  	if (node->ovflw_irq != 0) {
  		trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Reviewed-by Suzuki K Poulose <[email protected]>

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hanjun Guo

maintainer Reviewed-by on v1 2/4

2023-06-03 07:42 UTC #6 lore raw

Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

In reply to Robin Murphy

Quoted 27 lines · Robin Murphy

Don't register a resource for the second page unless the dual-page extension flag is actually present to say it's valid.
CC Lorenzo Pieralisi <[email protected]>
CC Hanjun Guo <[email protected]>
CC Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
Not a critical fix, since the driver currently works around this itself, but patch #4 would like to clean that up.
  drivers/acpi/arm64/apmt.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/arm64/apmt.c +1 -3
index 8cab69fa5d59..aa7d5c3c0dd8 100644
@@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
 
  	num_res++;
 
-	res[num_res].start = node->base_address1;
-	res[num_res].end = node->base_address1 + SZ_4K - 1;
-	res[num_res].flags = IORESOURCE_MEM;
+	if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {

drivers/acpi/arm64/apmt.c static int __init apmt_init_resources(struct resource *res, · line 47

Quoted 10 lines

drivers/acpi/arm64/apmt.c +5 -1

+		res[num_res].start = node->base_address1;
+		res[num_res].end = node->base_address1 + SZ_4K - 1;
+		res[num_res].flags = IORESOURCE_MEM;
 
-	num_res++;
+		num_res++;
+	}
 
  	if (node->ovflw_irq != 0) {
  		trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Good catch,

Reviewed-by Hanjun Guo <[email protected]>

Ilkka Koskinen

2023-06-05 07:31 UTC #7 lore raw

Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

In reply to Robin Murphy

Quoted 7 lines · Robin Murphy

Don't register a resource for the second page unless the dual-page extension flag is actually present to say it's valid.
CC Lorenzo Pieralisi <[email protected]>
CC Hanjun Guo <[email protected]>
CC Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

Reviewed-and-tested-by Ilkka Koskinen <[email protected]>

Quoted 34 lines

---

Not a critical fix, since the driver currently works around this itself, but patch #4 would like to clean that up.

drivers/acpi/arm64/apmt.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/arm64/apmt.c +6 -4

index 8cab69fa5d59..aa7d5c3c0dd8 100644
@@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
 
 	num_res++;
 
-	res[num_res].start = node->base_address1;
-	res[num_res].end = node->base_address1 + SZ_4K - 1;
-	res[num_res].flags = IORESOURCE_MEM;
+	if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
+		res[num_res].start = node->base_address1;
+		res[num_res].end = node->base_address1 + SZ_4K - 1;
+		res[num_res].flags = IORESOURCE_MEM;
 
-	num_res++;
+		num_res++;
+	}
 
 	if (node->ovflw_irq != 0) {
 		trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Robin Murphy

author

2023-06-01 12:00 UTC #8 lore raw

PATCH 3/4 perf/arm_cspmu: Clean up ACPI dependency

In reply to Robin Murphy

Build-wise, the ACPI dependency consists of only a couple of things which could probably stand being factored out into ACPI helpers anyway. However for the immediate concern of working towards Devicetree support here, it's easy enough to make a few tweaks to contain the affected code locally, such that we can relax the Kconfig dependency.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm_cspmu/Kconfig     |  3 +--
 drivers/perf/arm_cspmu/arm_cspmu.c | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

drivers/perf/arm_cspmu/Kconfig +1 -2

index 0b316fe69a45..25d25ded0983 100644
@@ -4,8 +4,7 @@
 
 config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
 	tristate "ARM Coresight Architecture PMU"
-	depends on ARM64 && ACPI
-	depends on ACPI_APMT || COMPILE_TEST
+	depends on ARM64 || COMPILE_TEST
 	help
 	  Provides support for performance monitoring unit (PMU) devices
 	  based on ARM CoreSight PMU architecture. Note that this PMU

drivers/perf/arm_cspmu/arm_cspmu.c +15 -2

index 72dc7a9e1ca8..3b91115c376d 100644
@@ -28,7 +28,6 @@
 #include <linux/module.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
-#include <acpi/processor.h>
 
 #include "arm_cspmu.h"
 #include "nvidia_cspmu.h"

@@ -1075,6 +1074,9 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
 	return 0;
 }
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_ARM64)
+#include <acpi/processor.h>
+
 static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
 {
 	u32 acpi_uid;

@@ -1099,7 +1101,7 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
 	return -ENODEV;
 }
 
-static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 {
 	struct device *dev;
 	struct acpi_apmt_node *apmt_node;

@@ -1135,6 +1137,17 @@ static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 
 	return 0;
 }
+#else
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
+{
+	return -ENODEV;
+}
+#endif
+
+static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+{
+	return arm_cspmu_acpi_get_cpus(cspmu);
+}
 
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 {

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Suzuki K Poulose

2023-06-02 16:02 UTC #9 lore raw

Re: [PATCH 3/4] perf/arm_cspmu: Clean up ACPI dependency

Quoted 65 lines · Robin Murphy

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm_cspmu/Kconfig     |  3 +--
  drivers/perf/arm_cspmu/arm_cspmu.c | 17 +++++++++++++++--
  2 files changed, 16 insertions(+), 4 deletions(-)

drivers/perf/arm_cspmu/Kconfig +1 -2

index 0b316fe69a45..25d25ded0983 100644
@@ -4,8 +4,7 @@
 
  config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
  	tristate "ARM Coresight Architecture PMU"
-	depends on ARM64 && ACPI
-	depends on ACPI_APMT || COMPILE_TEST
+	depends on ARM64 || COMPILE_TEST
  	help
  	  Provides support for performance monitoring unit (PMU) devices
  	  based on ARM CoreSight PMU architecture. Note that this PMU

drivers/perf/arm_cspmu/arm_cspmu.c +8 -2

index 72dc7a9e1ca8..3b91115c376d 100644
@@ -28,7 +28,6 @@
  #include <linux/module.h>
  #include <linux/perf_event.h>
  #include <linux/platform_device.h>
-#include <acpi/processor.h>
 
  #include "arm_cspmu.h"
  #include "nvidia_cspmu.h"

@@ -1075,6 +1074,9 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
  	return 0;
  }
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_ARM64)
+#include <acpi/processor.h>
+
  static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
  {
  	u32 acpi_uid;

@@ -1099,7 +1101,7 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
  	return -ENODEV;
  }
 
-static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
  {
  	struct device *dev;
  	struct acpi_apmt_node *apmt_node;

@@ -1135,6 +1137,17 @@ static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 
  	return 0;
  }
+#else
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
+{
+	return -ENODEV;

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu) · line 1153

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +7 -0

+}
+#endif
+
+static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+{
+	return arm_cspmu_acpi_get_cpus(cspmu);
+}
 
  static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
  {

Reviewed-and-Tested-by Suzuki K Poulose <[email protected]>

Ilkka Koskinen

Reviewed-by on v1 3/4

2023-06-05 07:33 UTC #10 lore raw

Re: [PATCH 3/4] perf/arm_cspmu: Clean up ACPI dependency

In reply to Robin Murphy

Quoted 7 lines · Robin Murphy

Build-wise, the ACPI dependency consists of only a couple of things which could probably stand being factored out into ACPI helpers anyway. However for the immediate concern of working towards Devicetree support here, it's easy enough to make a few tweaks to contain the affected code locally, such that we can relax the Kconfig dependency.
Signed-off-by Robin Murphy <[email protected]>

Reviewed-by Ilkka Koskinen <[email protected]>

Quoted 72 lines

---
drivers/perf/arm_cspmu/Kconfig | 3 +--
drivers/perf/arm_cspmu/arm_cspmu.c | 17 +++++++++++++++--

2 files changed, 16 insertions(+), 4 deletions(-)

drivers/perf/arm_cspmu/Kconfig +0 -0

index 0b316fe69a45..25d25ded0983 100644
@@ -4,8 +4,7 @@

config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
tristate "ARM Coresight Architecture PMU"

drivers/perf/arm_cspmu/Kconfig +1 -2

-	depends on ARM64 && ACPI
-	depends on ACPI_APMT || COMPILE_TEST
+	depends on ARM64 || COMPILE_TEST

help
Provides support for performance monitoring unit (PMU) devices based on ARM CoreSight PMU architecture. Note that this PMU

drivers/perf/arm_cspmu/arm_cspmu.c +15 -2

index 72dc7a9e1ca8..3b91115c376d 100644
@@ -28,7 +28,6 @@
 #include <linux/module.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
-#include <acpi/processor.h>
 
 #include "arm_cspmu.h"
 #include "nvidia_cspmu.h"

@@ -1075,6 +1074,9 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
 	return 0;
 }
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_ARM64)
+#include <acpi/processor.h>
+
 static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
 {
 	u32 acpi_uid;

@@ -1099,7 +1101,7 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
 	return -ENODEV;
 }
 
-static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 {
 	struct device *dev;
 	struct acpi_apmt_node *apmt_node;

@@ -1135,6 +1137,17 @@ static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 
 	return 0;
 }
+#else
+static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
+{
+	return -ENODEV;
+}
+#endif
+
+static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
+{
+	return arm_cspmu_acpi_get_cpus(cspmu);
+}
 
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 {

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Robin Murphy

author

2023-06-01 12:00 UTC #11 lore raw

PATCH 4/4 perf/arm_cspmu: Decouple APMT dependency

In reply to Robin Murphy

The functional paths of the driver need not care about ACPI, so abstract the property of atomic doubleword access as its own flag (repacking the structure for a better fit). We also do not need to go poking directly at the APMT for standard resources which the ACPI layer has already dealt with, so deal with the optional MMIO page and interrupt in the normal firmware-agnostic manner. The few remaining portions of probing that *are* APMT-specific can still easily retrieve the APMT pointer as needed without us having to carry a duplicate copy around everywhere.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
 drivers/perf/arm_cspmu/arm_cspmu.h |  4 +--
 2 files changed, 13 insertions(+), 36 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +11 ~~-34~~

index 3b91115c376d..f8daf252a488 100644
@@ -100,10 +100,6 @@
 #define ARM_CSPMU_ACTIVE_CPU_MASK		0x0
 #define ARM_CSPMU_ASSOCIATED_CPU_MASK		0x1
 
-/* Check if field f in flags is set with value v */
-#define CHECK_APMT_FLAG(flags, f, v) \
-	((flags & (ACPI_APMT_FLAGS_ ## f)) == (ACPI_APMT_FLAGS_ ## f ## _ ## v))
-
 /* Check and use default if implementer doesn't provide attribute callback */
 #define CHECK_DEFAULT_IMPL_OPS(ops, callback)			\
 	do {							\

@@ -155,12 +151,6 @@ static u64 read_reg64_hilohi(const void __iomem *addr, u32 max_poll_count)
 	return val;
 }
 
-/* Check if PMU supports 64-bit single copy atomic. */
-static inline bool supports_64bit_atomics(const struct arm_cspmu *cspmu)
-{
-	return CHECK_APMT_FLAG(cspmu->apmt_node->flags, ATOMIC, SUPP);
-}
-
 /* Check if cycle counter is supported. */
 static inline bool supports_cycle_counter(const struct arm_cspmu *cspmu)
 {

@@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
 	static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
 
 	dev = cspmu->dev;
-	apmt_node = cspmu->apmt_node;
+	apmt_node = dev_get_platdata(dev);
 	pmu_type = apmt_node->type;
 
 	if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {

@@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
 	int ret;
-	struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
 	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+	struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);
 	const struct impl_match *match = impl_match;
 
 	/*

@@ -719,7 +709,7 @@ static u64 arm_cspmu_read_counter(struct perf_event *event)
 		offset = counter_offset(sizeof(u64), event->hw.idx);
 		counter_addr = cspmu->base1 + offset;
 
-		return supports_64bit_atomics(cspmu) ?
+		return cspmu->has_atomic_dword ?
 			       readq(counter_addr) :
 			       read_reg64_hilohi(counter_addr, HILOHI_MAX_POLL);
 	}

@@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 {
 	struct acpi_apmt_node *apmt_node;
 	struct arm_cspmu *cspmu;
-	struct device *dev;
-
-	dev = &pdev->dev;
-	apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
-	if (!apmt_node) {
-		dev_err(dev, "failed to get APMT node\n");
-		return NULL;
-	}
+	struct device *dev = &pdev->dev;
 
 	cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
 	if (!cspmu)
 		return NULL;
 
 	cspmu->dev = dev;
-	cspmu->apmt_node = apmt_node;
-
 	platform_set_drvdata(pdev, cspmu);
 
+	apmt_node = dev_get_platdata(dev);
+	cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+
 	return cspmu;
 }

@@ -935,11 +919,9 @@ static int arm_cspmu_init_mmio(struct arm_cspmu *cspmu)
 {
 	struct device *dev;
 	struct platform_device *pdev;
-	struct acpi_apmt_node *apmt_node;
 
 	dev = cspmu->dev;
 	pdev = to_platform_device(dev);
-	apmt_node = cspmu->apmt_node;
 
 	/* Base address for page 0. */
 	cspmu->base0 = devm_platform_ioremap_resource(pdev, 0);

@@ -950,7 +932,7 @@ static int arm_cspmu_init_mmio(struct arm_cspmu *cspmu)
 
 	/* Base address for page 1 if supported. Otherwise point to page 0. */
 	cspmu->base1 = cspmu->base0;
-	if (CHECK_APMT_FLAG(apmt_node->flags, DUAL_PAGE, SUPP)) {
+	if (platform_get_resource(pdev, IORESOURCE_MEM, 1)) {
 		cspmu->base1 = devm_platform_ioremap_resource(pdev, 1);
 		if (IS_ERR(cspmu->base1)) {
 			dev_err(dev, "ioremap failed for page-1 resource\n");

@@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
 	int irq, ret;
 	struct device *dev;
 	struct platform_device *pdev;
-	struct acpi_apmt_node *apmt_node;
 
 	dev = cspmu->dev;
 	pdev = to_platform_device(dev);
-	apmt_node = cspmu->apmt_node;
 
 	/* Skip IRQ request if the PMU does not support overflow interrupt. */
-	if (apmt_node->ovflw_irq == 0)
-		return 0;
-
-	irq = platform_get_irq(pdev, 0);
+	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0)
-		return irq;
+		return irq == -ENXIO ? 0 : irq;
 
 	ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
 			       IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),

@@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 	int cpu;
 
 	dev = cspmu->pmu.dev;
-	apmt_node = cspmu->apmt_node;
+	apmt_node = dev_get_platdata(dev);
 	affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;
 
 	if (affinity_flag == ACPI_APMT_FLAGS_AFFINITY_PROC) {

drivers/perf/arm_cspmu/arm_cspmu.h +2 -2

index 51323b175a4a..7892e587f606 100644
@@ -118,16 +118,16 @@ struct arm_cspmu_impl {
 struct arm_cspmu {
 	struct pmu pmu;
 	struct device *dev;
-	struct acpi_apmt_node *apmt_node;
 	const char *name;
 	const char *identifier;
 	void __iomem *base0;
 	void __iomem *base1;
-	int irq;
 	cpumask_t associated_cpus;
 	cpumask_t active_cpu;
 	struct hlist_node cpuhp_node;
+	int irq;
 
+	bool has_atomic_dword;
 	u32 pmcfgr;
 	u32 num_logical_ctrs;
 	u32 num_set_clr_reg;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Suzuki K Poulose

Reviewed-by on v1 4/4

2023-06-02 10:55 UTC #12 lore raw

Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

Quoted 10 lines · Robin Murphy

The functional paths of the driver need not care about ACPI, so abstract the property of atomic doubleword access as its own flag (repacking the structure for a better fit). We also do not need to go poking directly at the APMT for standard resources which the ACPI layer has already dealt with, so deal with the optional MMIO page and interrupt in the normal firmware-agnostic manner. The few remaining portions of probing that *are* APMT-specific can still easily retrieve the APMT pointer as needed without us having to carry a duplicate copy around everywhere.
Signed-off-by Robin Murphy <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

Ilkka Koskinen

2023-06-05 07:13 UTC #13 lore raw

Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

In reply to Robin Murphy

Hi Robin,

I have a couple of comments below

Quoted 19 lines · Robin Murphy

The functional paths of the driver need not care about ACPI, so abstract the property of atomic doubleword access as its own flag (repacking the structure for a better fit). We also do not need to go poking directly at the APMT for standard resources which the ACPI layer has already dealt with, so deal with the optional MMIO page and interrupt in the normal firmware-agnostic manner. The few remaining portions of probing that *are* APMT-specific can still easily retrieve the APMT pointer as needed without us having to carry a duplicate copy around everywhere.
Signed-off-by Robin Murphy <[email protected]>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++---------------------- drivers/perf/arm_cspmu/arm_cspmu.h | 4 +--
2 files changed, 13 insertions(+), 36 deletions(-)
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
index 3b91115c376d..f8daf252a488 100644

...

drivers/perf/arm_cspmu/arm_cspmu.c static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu) · line 312

Quoted 6 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -1

@@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
 	static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
 
 	dev = cspmu->dev;
-	apmt_node = cspmu->apmt_node;
+	apmt_node = dev_get_platdata(dev);

Was platdata changed too? If not, I think this should be

	apmt_node = *(struct acpi_apmt_node **) dev_get_platdata(dev);

drivers/perf/arm_cspmu/arm_cspmu.c static const struct impl_match impl_match[] = { · line 390

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -1

 	pmu_type = apmt_node->type;
 
 	if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {

@@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
 	int ret;
-	struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
 	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+	struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);

Ditto

Quoted 3 lines

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
 	const struct impl_match *match = impl_match;
 
 	/*

...

Quoted 14 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -8

@@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 {
 	struct acpi_apmt_node *apmt_node;
 	struct arm_cspmu *cspmu;
-	struct device *dev;
-
-	dev = &pdev->dev;
-	apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
-	if (!apmt_node) {
-		dev_err(dev, "failed to get APMT node\n");
-		return NULL;
-	}
+	struct device *dev = &pdev->dev;

drivers/perf/arm_cspmu/arm_cspmu.c static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev) · line 912

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -2

 	cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
 	if (!cspmu)
 		return NULL;
 
 	cspmu->dev = dev;
-	cspmu->apmt_node = apmt_node;
-
 	platform_set_drvdata(pdev, cspmu);
 
+	apmt_node = dev_get_platdata(dev);

Ditto

Quoted 4 lines

drivers/perf/arm_cspmu/arm_cspmu.c +2 -0

+	cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+
 	return cspmu;
 }

...

Quoted 23 lines

drivers/perf/arm_cspmu/arm_cspmu.c +2 -7

@@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
 	int irq, ret;
 	struct device *dev;
 	struct platform_device *pdev;
-	struct acpi_apmt_node *apmt_node;
 
 	dev = cspmu->dev;
 	pdev = to_platform_device(dev);
-	apmt_node = cspmu->apmt_node;
 
 	/* Skip IRQ request if the PMU does not support overflow interrupt. */
-	if (apmt_node->ovflw_irq == 0)
-		return 0;
-
-	irq = platform_get_irq(pdev, 0);
+	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0)
-		return irq;
+		return irq == -ENXIO ? 0 : irq;
 
 	ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
 			       IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) · line 1088

Quoted 3 lines

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0

@@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 	int cpu;
 
 	dev = cspmu->pmu.dev;

You didn't touch this one but shouldn't this be

	dev = cspmu->dev;

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) · line 1089

Quoted 2 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -1
-	apmt_node = cspmu->apmt_node;
+	apmt_node = dev_get_platdata(dev);

Ditto

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) · line 1090

Quoted 1 line

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
 	affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;

Otherwise the patch looks good to me.

Cheers, Ilkka

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Robin Murphy

author

2023-06-05 10:49 UTC #14 lore raw

Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

Quoted 32 lines · Ilkka Koskinen

Hi Robin,
I have a couple of comments below
Quoted 19 lines · Robin Murphy
The functional paths of the driver need not care about ACPI, so abstract the property of atomic doubleword access as its own flag (repacking the structure for a better fit). We also do not need to go poking directly at the APMT for standard resources which the ACPI layer has already dealt with, so deal with the optional MMIO page and interrupt in the normal firmware-agnostic manner. The few remaining portions of probing that *are* APMT-specific can still easily retrieve the APMT pointer as needed without us having to carry a duplicate copy around everywhere.
Signed-off-by Robin Murphy <[email protected]>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++---------------------- drivers/perf/arm_cspmu/arm_cspmu.h | 4 +--
2 files changed, 13 insertions(+), 36 deletions(-)
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
index 3b91115c376d..f8daf252a488 100644
...
Quoted 4 lines
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
@@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct 
 arm_cspmu *cspmu)
     static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
 

Quoted 7 lines

drivers/perf/arm_cspmu/arm_cspmu.c static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu) · line 312
Quoted 3 lines
drivers/perf/arm_cspmu/arm_cspmu.c +1 -1
     dev = cspmu->dev;
-    apmt_node = cspmu->apmt_node;
+    apmt_node = dev_get_platdata(dev);
Was platdata changed too? If not, I think this should be
apmt_node = *(struct acpi_apmt_node **) dev_get_platdata(dev);

Oof, indeed it looks like I got the wrong thing into my head at the critical moment :(

Clearly this deserves a nice helpful wrapper so we only have to think about the nasty casting once...

Quoted 81 lines

drivers/perf/arm_cspmu/arm_cspmu.c static const struct impl_match impl_match[] = { · line 390

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0

     pmu_type = apmt_node->type;

if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {

drivers/perf/arm_cspmu/arm_cspmu.c +1 -1

@@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
     int ret;
-    struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
     struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+    struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);

Ditto

Quoted 3 lines

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0

     const struct impl_match *match = impl_match;
 
     /*

...

Quoted 15 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -8

@@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct 
 platform_device *pdev)
 {
     struct acpi_apmt_node *apmt_node;
     struct arm_cspmu *cspmu;
-    struct device *dev;
-
-    dev = &pdev->dev;
-    apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
-    if (!apmt_node) {
-        dev_err(dev, "failed to get APMT node\n");
-        return NULL;
-    }
+    struct device *dev = &pdev->dev;

drivers/perf/arm_cspmu/arm_cspmu.c static struct arm_cspmu *arm_cspmu_alloc(struct · line 913

Quoted 10 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -2

     cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
     if (!cspmu)
         return NULL;
 
     cspmu->dev = dev;
-    cspmu->apmt_node = apmt_node;
-
     platform_set_drvdata(pdev, cspmu);
 
+    apmt_node = dev_get_platdata(dev);

Ditto

Quoted 4 lines

drivers/perf/arm_cspmu/arm_cspmu.c +2 -0

+    cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+
     return cspmu;
 }

...

Quoted 27 lines

drivers/perf/arm_cspmu/arm_cspmu.c +2 -7

@@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct 
 arm_cspmu *cspmu)
     int irq, ret;
     struct device *dev;
     struct platform_device *pdev;
-    struct acpi_apmt_node *apmt_node;
 
     dev = cspmu->dev;
     pdev = to_platform_device(dev);
-    apmt_node = cspmu->apmt_node;
 
     /* Skip IRQ request if the PMU does not support overflow interrupt. */
-    if (apmt_node->ovflw_irq == 0)
-        return 0;
-
-    irq = platform_get_irq(pdev, 0);
+    irq = platform_get_irq_optional(pdev, 0);
     if (irq < 0)
-        return irq;
+        return irq == -ENXIO ? 0 : irq;
 
     ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,

IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0

@@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct 
 arm_cspmu *cspmu)
     int cpu;

Quoted 5 lines

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) · line 1088
Quoted 1 line
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
     dev = cspmu->pmu.dev;
You didn't touch this one but shouldn't this be
dev = cspmu->dev;

Good catch - attributing the error message to the wrong device doesn't matter too much currently, but this change does then need it to be right. Will fix that too.

Thanks!
Robin.

Quoted 12 lines

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct · line 1090
Quoted 3 lines
drivers/perf/arm_cspmu/arm_cspmu.c +1 -1
-    apmt_node = cspmu->apmt_node;
+    apmt_node = dev_get_platdata(dev);
Ditto
drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) · line 1090
Quoted 1 line
drivers/perf/arm_cspmu/arm_cspmu.c +0 -0
     affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;
Otherwise the patch looks good to me.
Cheers, Ilkka