arm_pmu: fix lockdep issues with ACPI systems

In ACPI systems, we don't know the makeup of CPUs until we hotplug them on, and thus have to allocate the PMU datastrcutures at hotplug time. Thus, we must use GFP_ATOMIC allocations.

Reorganise the PMU allocators to take a GFP argument so that we can permit this.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c          | 6 +++---
 drivers/perf/arm_pmu_acpi.c     | 2 +-
 drivers/perf/arm_pmu_platform.c | 2 +-
 include/linux/perf/arm_pmu.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

drivers/perf/arm_pmu.c +3 -3

index 57df8dce8e19..3d6d4c5f2356 100644
@@ -779,18 +779,18 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 					    &cpu_pmu->node);
 }
 
-struct arm_pmu *armpmu_alloc(void)
+struct arm_pmu *armpmu_alloc(gfp_t flags)
 {
 	struct arm_pmu *pmu;
 	int cpu;
 
-	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
+	pmu = kzalloc(sizeof(*pmu), flags);
 	if (!pmu) {
 		pr_info("failed to allocate PMU device!\n");
 		goto out;
 	}
 
-	pmu->hw_events = alloc_percpu(struct pmu_hw_events);
+	pmu->hw_events = alloc_percpu_gfp(struct pmu_hw_events, flags);
 	if (!pmu->hw_events) {
 		pr_info("failed to allocate per-cpu PMU data.\n");
 		goto out_free_pmu;

drivers/perf/arm_pmu_acpi.c +1 -1

index 705f1a390e31..a52f5b673a15 100644
@@ -127,7 +127,7 @@ static struct arm_pmu *arm_pmu_acpi_find_alloc_pmu(void)
 		return pmu;
 	}
 
-	pmu = armpmu_alloc();
+	pmu = armpmu_alloc(GFP_ATOMIC);
 	if (!pmu) {
 		pr_warn("Unable to allocate PMU for CPU%d\n",
 			smp_processor_id());

drivers/perf/arm_pmu_platform.c +1 -1

index db863f14c406..686b3f28c5d1 100644
@@ -203,7 +203,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	struct arm_pmu *pmu;
 	int ret = -ENODEV;
 
-	pmu = armpmu_alloc();
+	pmu = armpmu_alloc(GFP_KERNEL);
 	if (!pmu)
 		return -ENOMEM;
 

include/linux/perf/arm_pmu.h +1 -1

index 97426407904d..52e6d56a6a4e 100644
@@ -173,7 +173,7 @@ static inline int arm_pmu_acpi_probe(armpmu_init_fn init_fn) { return 0; }
 #endif
 
 /* Internal functions only for core arm_pmu code */
-struct arm_pmu *armpmu_alloc(void);
+struct arm_pmu *armpmu_alloc(gfp_t flags);
 void armpmu_free(struct arm_pmu *pmu);
 int armpmu_register(struct arm_pmu *pmu);
 int armpmu_request_irq(struct arm_pmu *armpmu, int cpu);

In ACPI systems, we don't know the makeup of CPUs until we hotplug them on, and thus have to allocate the PMU datastrcutures at hotplug time. Thus, we must use GFP_ATOMIC allocations.

Reorganise the PMU allocators to take a GFP argument so that we can permit this.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c          | 6 +++---
 drivers/perf/arm_pmu_acpi.c     | 2 +-
 drivers/perf/arm_pmu_platform.c | 2 +-
 include/linux/perf/arm_pmu.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

drivers/perf/arm_pmu.c +3 -3

index 57df8dce8e19..3d6d4c5f2356 100644
@@ -779,18 +779,18 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 					    &cpu_pmu->node);
 }
 
-struct arm_pmu *armpmu_alloc(void)
+struct arm_pmu *armpmu_alloc(gfp_t flags)
 {
 	struct arm_pmu *pmu;
 	int cpu;
 
-	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
+	pmu = kzalloc(sizeof(*pmu), flags);
 	if (!pmu) {
 		pr_info("failed to allocate PMU device!\n");
 		goto out;
 	}
 
-	pmu->hw_events = alloc_percpu(struct pmu_hw_events);
+	pmu->hw_events = alloc_percpu_gfp(struct pmu_hw_events, flags);
 	if (!pmu->hw_events) {
 		pr_info("failed to allocate per-cpu PMU data.\n");
 		goto out_free_pmu;

drivers/perf/arm_pmu_acpi.c +0 -0

index 705f1a390e31..a52f5b673a15 100644

Quoted 8 lines · Mark Rutland

drivers/perf/arm_pmu_acpi.c +0 -0

index 705f1a390e31..a52f5b673a15 100644
@@ -127,7 +127,7 @@ static struct arm_pmu *arm_pmu_acpi_find_alloc_pmu(void)
 		return pmu;
 	}
 

drivers/perf/arm_pmu_acpi.c static struct arm_pmu *arm_pmu_acpi_find_alloc_pmu(void) · line 130

Quoted 2 lines

drivers/perf/arm_pmu_acpi.c +1 -1

-	pmu = armpmu_alloc();
+	pmu = armpmu_alloc(GFP_ATOMIC);

I think I'd rather have armpmu_alloc_atomic as a wrapper around __armpmu_alloc(GFP_ATOMIC) and then leave the armpmu_alloc to pass GFP_KERNEL.

The arm_pmu platform code explicitly checks for mismatched PPIs at probe time, while the ACPI code leaves this to the core code. Future refactoring will make this difficult for the core code to check, so let's have the ACPI code check this explicitly.

As before, upon a failure we'll continue on without an interrupt. Ho hum.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c      | 16 ++++------------
 drivers/perf/arm_pmu_acpi.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 16 deletions(-)

drivers/perf/arm_pmu.c +0 -4

index 3d6d4c5f2356..e0242103d904 100644
@@ -557,18 +557,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
 	if (!irq)
 		return 0;
 
-	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
-		err = request_percpu_irq(irq, handler, "arm-pmu",
-					 &hw_events->percpu_pmu);
-	} else if (irq_is_percpu_devid(irq)) {

Quoted 23 lines · Mark Rutland

The arm_pmu platform code explicitly checks for mismatched PPIs at probe time, while the ACPI code leaves this to the core code. Future refactoring will make this difficult for the core code to check, so let's have the ACPI code check this explicitly.

As before, upon a failure we'll continue on without an interrupt. Ho hum.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c      | 16 ++++------------
 drivers/perf/arm_pmu_acpi.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 16 deletions(-)

drivers/perf/arm_pmu.c +0 -0

index 3d6d4c5f2356..e0242103d904 100644
@@ -557,18 +557,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
 	if (!irq)
 		return 0;
 

Quoted 4 lines

drivers/perf/arm_pmu.c +0 -4

-	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
-		err = request_percpu_irq(irq, handler, "arm-pmu",
-					 &hw_events->percpu_pmu);
-	} else if (irq_is_percpu_devid(irq)) {

drivers/perf/arm_pmu.c int armpmu_request_irq(struct arm_pmu *armpmu, int cpu) · line 561

Quoted 10 lines

drivers/perf/arm_pmu.c +2 -8

-		int other_cpu = cpumask_first(&armpmu->active_irqs);
-		int other_irq = per_cpu(hw_events->irq, other_cpu);
-
-		if (irq != other_irq) {
-			pr_warn("mismatched PPIs detected.\n");
-			err = -EINVAL;
-			goto err_out;
-		}
+	if (irq_is_percpu_devid(irq)) {
+		if (cpumask_empty(&armpmu->active_irqs))

Why not leave this as before, with a '&&' operator?

Quoted 24 lines · Will Deacon

Quoted 23 lines · Mark Rutland

The arm_pmu platform code explicitly checks for mismatched PPIs at probe time, while the ACPI code leaves this to the core code. Future refactoring will make this difficult for the core code to check, so let's have the ACPI code check this explicitly.

As before, upon a failure we'll continue on without an interrupt. Ho hum.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c      | 16 ++++------------
 drivers/perf/arm_pmu_acpi.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 16 deletions(-)

drivers/perf/arm_pmu.c +0 -0

index 3d6d4c5f2356..e0242103d904 100644
@@ -557,18 +557,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
 	if (!irq)
 		return 0;
 

Quoted 16 lines

Quoted 4 lines

drivers/perf/arm_pmu.c +0 -4

-	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
-		err = request_percpu_irq(irq, handler, "arm-pmu",
-					 &hw_events->percpu_pmu);
-	} else if (irq_is_percpu_devid(irq)) {

drivers/perf/arm_pmu.c int armpmu_request_irq(struct arm_pmu *armpmu, int cpu) · line 561

Quoted 10 lines

drivers/perf/arm_pmu.c +2 -8

-		int other_cpu = cpumask_first(&armpmu->active_irqs);
-		int other_irq = per_cpu(hw_events->irq, other_cpu);
-
-		if (irq != other_irq) {
-			pr_warn("mismatched PPIs detected.\n");
-			err = -EINVAL;
-			goto err_out;
-		}
+	if (irq_is_percpu_devid(irq)) {
+		if (cpumask_empty(&armpmu->active_irqs))

Why not leave this as before, with a '&&' operator?

Because then we'd fall into the else case (for SPIs), were the active_irqs mask empty.

Previously, that would have been caught by the irq_is_percpu_devid(irq) case that got removed.

I can instead make this:

	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
		err = request_percpu_irq(irq, handler, "arm-pmu",
					 &hw_events->percpu_pmu);
	} else if (irq_is_percpu_devid(irq)) {
		/* nothing to do */
	} else  { 
		< SPI case >
	}

... but that seemed more painful to read.

The current way we manage IRQs forces the ACPI PMU driver to request IRQs in the cpu bringup path, which isn't safe due to implicit memory allocations in the request_irq() path.

To solve that, we need to decouple requesting IRQs from PMU management, requesting IRQs up-front, before we know the associated PMU. We will separately (and perhaps later) associate each IRQ with its PMU.

This patch allows the IRQ handlers to be registered without a PMU dev argument, using a percpu pointer instead.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c       | 93 ++++++++++++++++++++++++++++----------------
 include/linux/perf/arm_pmu.h |  3 +-
 2 files changed, 62 insertions(+), 34 deletions(-)

drivers/perf/arm_pmu.c +19 -11

index e0242103d904..287b3edfb4cc 100644
@@ -26,6 +26,9 @@
 
 #include <asm/irq_regs.h>
 
+static DEFINE_PER_CPU(struct arm_pmu *, cpu_armpmu);
+static DEFINE_PER_CPU(int, cpu_irq);
+
 static int
 armpmu_map_cache_event(const unsigned (*cache_map)
 				      [PERF_COUNT_HW_CACHE_MAX]

@@ -334,13 +337,9 @@ static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
 	int ret;
 	u64 start_clock, finish_clock;
 
-	/*
-	 * we request the IRQ with a (possibly percpu) struct arm_pmu**, but
-	 * the handlers expect a struct arm_pmu*. The percpu_irq framework will
-	 * do any necessary shifting, we just need to perform the first
-	 * dereference.
-	 */
-	armpmu = *(void **)dev;
+	armpmu = this_cpu_read(cpu_armpmu);
+	if (WARN_ON_ONCE(!armpmu))
+		return IRQ_NONE;
 
 	plat = armpmu_get_platdata(armpmu);
 

@@ -531,40 +530,56 @@ int perf_num_counters(void)
 }
 EXPORT_SYMBOL_GPL(perf_num_counters);
 
-void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
+int armpmu_count_irq_users(const int irq)
 {
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);
+	int cpu, count = 0;
+
+	for_each_possible_cpu(cpu) {
+		if (per_cpu(cpu_irq, cpu) == irq)
+			count++;
+	}
+
+	return count;
+}
 
-	if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
+void __armpmu_free_irq(int irq, int cpu)
+{
+	if (per_cpu(cpu_irq, cpu) == 0)

Quoted 27 lines · Mark Rutland

drivers/perf/arm_pmu.c +15 -4

@@ -531,40 +530,56 @@ int perf_num_counters(void)
 }
 EXPORT_SYMBOL_GPL(perf_num_counters);
 
-void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
+int armpmu_count_irq_users(const int irq)
 {
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);
+	int cpu, count = 0;
+
+	for_each_possible_cpu(cpu) {
+		if (per_cpu(cpu_irq, cpu) == irq)
+			count++;
+	}
+
+	return count;
+}
 
-	if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
+void __armpmu_free_irq(int irq, int cpu)
+{
+	if (per_cpu(cpu_irq, cpu) == 0)
+		return;
+	if (WARN_ON(irq != per_cpu(cpu_irq, cpu)))
 		return;
 

drivers/perf/arm_pmu.c int perf_num_counters(void) · line 554

Quoted 6 lines

drivers/perf/arm_pmu.c +2 -3

 	if (irq_is_percpu_devid(irq)) {
-		free_percpu_irq(irq, &hw_events->percpu_pmu);
-		cpumask_clear(&armpmu->active_irqs);
-		return;
+		if (armpmu_count_irq_users(irq) == 1)
+			free_percpu_irq(irq, &cpu_armpmu);

Do you actually need the count, or could you just free the irq the first time this is called and set all of the cpu_irqs to 0?

We can't request IRQs in atomic context, so for ACPI systems we'll have to request them up-front, and later associate them with CPUs.

This patch reorganises the arm_pmu code to do so. As we no longer have the arm_pmu strucutre at probe time, a number of prototypes need to be adjusted, requiring changes to the common arm_pmu code and arm_pmu platform code.

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

Cc Will Deacon <[email protected]>

---
 drivers/perf/arm_pmu.c          | 37 +++----------------------------------
 drivers/perf/arm_pmu_acpi.c     | 20 +++++++-------------
 drivers/perf/arm_pmu_platform.c | 25 ++++++++++++++++++++++---
 include/linux/perf/arm_pmu.h    | 13 +++++++++++--
 4 files changed, 43 insertions(+), 52 deletions(-)

drivers/perf/arm_pmu.c +3 -23

index 287b3edfb4cc..534b4b3fb440 100644
@@ -323,13 +323,6 @@ validate_group(struct perf_event *event)
 	return 0;
 }
 
-static struct arm_pmu_platdata *armpmu_get_platdata(struct arm_pmu *armpmu)
-{
-	struct platform_device *pdev = armpmu->plat_device;
-
-	return pdev ? dev_get_platdata(&pdev->dev) : NULL;
-}
-
 static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
 {
 	struct arm_pmu *armpmu;

@@ -542,7 +535,7 @@ int armpmu_count_irq_users(const int irq)
 	return count;
 }
 
-void __armpmu_free_irq(int irq, int cpu)
+void armpmu_free_irq(int irq, int cpu)
 {
 	if (per_cpu(cpu_irq, cpu) == 0)
 		return;

@@ -559,16 +552,7 @@ void __armpmu_free_irq(int irq, int cpu)
 	per_cpu(cpu_irq, cpu) = 0;
 }
 
-void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
-{
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);
-
-	__armpmu_free_irq(irq, cpu);
-	per_cpu(cpu_armpmu, cpu) = NULL;
-}
-
-int armpmu_request_irq_flags(int irq, unsigned long irq_flags, int cpu)
+int armpmu_request_irq(int irq, unsigned long irq_flags, int cpu)
 {
 	int err = 0;
 	const irq_handler_t handler = armpmu_dispatch_irq;

@@ -603,24 +587,9 @@ int armpmu_request_irq_flags(int irq, unsigned long irq_flags, int cpu)
 	return err;
 }
 
-int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
+void armpmu_bind_cpu(struct arm_pmu *armpmu, int cpu)
 {
-	struct arm_pmu_platdata *platdata = armpmu_get_platdata(armpmu);
-	unsigned long irq_flags;
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);

Quoted 18 lines · Mark Rutland

We can't request IRQs in atomic context, so for ACPI systems we'll have to request them up-front, and later associate them with CPUs.

This patch reorganises the arm_pmu code to do so. As we no longer have the arm_pmu strucutre at probe time, a number of prototypes need to be adjusted, requiring changes to the common arm_pmu code and arm_pmu platform code.

drivers/perf/arm_pmu.c +1 -8

+void armpmu_bind_cpu(struct arm_pmu *armpmu, int cpu)
 {
-	struct arm_pmu_platdata *platdata = armpmu_get_platdata(armpmu);
-	unsigned long irq_flags;
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);
-	if (!irq)
-		return 0;
-
-	if (platdata && platdata->irq_flags) {

drivers/perf/arm_pmu.c int armpmu_request_irq_flags(int irq, unsigned long irq_flags, int cpu) · line 592

Quoted 6 lines

drivers/perf/arm_pmu.c +0 -5

-		irq_flags = platdata->irq_flags;
-	} else {
-		irq_flags = ARM_PMU_IRQ_FLAGS;
-	}
-
 	per_cpu(cpu_armpmu, cpu) = armpmu;

Can we not make the binding implicit in armpmu_{request,free}_irq?

Quoted 19 lines · Will Deacon

Quoted 18 lines · Mark Rutland

We can't request IRQs in atomic context, so for ACPI systems we'll have to request them up-front, and later associate them with CPUs.

This patch reorganises the arm_pmu code to do so. As we no longer have the arm_pmu strucutre at probe time, a number of prototypes need to be adjusted, requiring changes to the common arm_pmu code and arm_pmu platform code.

drivers/perf/arm_pmu.c +1 -8

+void armpmu_bind_cpu(struct arm_pmu *armpmu, int cpu)
 {
-	struct arm_pmu_platdata *platdata = armpmu_get_platdata(armpmu);
-	unsigned long irq_flags;
-	struct pmu_hw_events __percpu *hw_events = armpmu->hw_events;
-	int irq = per_cpu(hw_events->irq, cpu);
-	if (!irq)
-		return 0;
-
-	if (platdata && platdata->irq_flags) {

Quoted 8 lines

drivers/perf/arm_pmu.c int armpmu_request_irq_flags(int irq, unsigned long irq_flags, int cpu) · line 592

Quoted 6 lines

drivers/perf/arm_pmu.c +0 -5

-		irq_flags = platdata->irq_flags;
-	} else {
-		irq_flags = ARM_PMU_IRQ_FLAGS;
-	}
-
 	per_cpu(cpu_armpmu, cpu) = armpmu;

Can we not make the binding implicit in armpmu_{request,free}_irq?

Unfortunately not.

As mentioned in the commit message (typo and all), in the ACPI case, we need to request/free IRQs before we know the PMU.

Currently the arm_pmu ACPI code is somewhat dubious. It attempts ot allocate memory any manpiulate IRQs in a hotplug callback, which is an atomic context.

These patches (based on the arm64 for-next/core branch [1]) attempt to address this by moving work out of hotplug callback, requiring a reorganisation of the common arm_pmu code.

I've given these a boot-test on a Juno R1 system, both with DT and ACPI. In either case the PMU works as expected, and lockdep seems happy.

Currently the arm_pmu ACPI code is somewhat dubious. It attempts ot allocate memory any manpiulate IRQs in a hotplug callback, which is an atomic context.

These patches (based on the arm64 for-next/core branch [1]) attempt to address this by moving work out of hotplug callback, requiring a reorganisation of the common arm_pmu code.

I've given these a boot-test on a Juno R1 system, both with DT and ACPI. In either case the PMU works as expected, and lockdep seems happy.

I've pushed the series out to my arm64/acpi-pmu-lockdep branch [2].