Re: [PATCH v3] perf: arm_dsu: Support DSU ACPI devices

[Suzuki K Poulose]

On 09/07/2020 12:02 PM, Will Deacon wrote:
[+ Suzuki as I'd like his Ack on this]

Thanks Will !

On Fri, Aug 14, 2020 at 05:39:40PM -0700, Tuan Phan wrote:
Add support for probing device from ACPI node.
Each DSU ACPI node and its associated cpus are inside a cluster node.

Signed-off-by: Tuan Phan <tuanp...@os.amperecomputing.com>
---
Changes in v3:
- Based on the latest ARM ACPI binding at: 
https://developer.arm.com/documentation/den0093/c/

Changes in v2:
- Removed IRQF_SHARED.
- Fixed ACPI runtime detection.

  drivers/perf/arm_dsu_pmu.c | 68 ++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c
index 96ed93c..4be355d 100644
--- a/drivers/perf/arm_dsu_pmu.c
+++ b/drivers/perf/arm_dsu_pmu.c
@@ -11,6 +11,7 @@
  #define DRVNAME               PMUNAME "_pmu"
  #define pr_fmt(fmt)   DRVNAME ": " fmt
  
+#include <linux/acpi.h>
  #include <linux/bitmap.h>
  #include <linux/bitops.h>
  #include <linux/bug.h>
@@ -603,18 +604,21 @@ static struct dsu_pmu *dsu_pmu_alloc(struct 
platform_device *pdev)
  }
  
  /**
- * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
+ * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster
+ * from device tree.
   */
-static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
+static int dsu_pmu_dt_get_cpus(struct platform_device *pdev)
  {
        int i = 0, n, cpu;
        struct device_node *cpu_node;
+       struct dsu_pmu *dsu_pmu =
+               (struct dsu_pmu *) platform_get_drvdata(pdev);
  
-	n = of_count_phandle_with_args(dev, "cpus", NULL);
+       n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
        if (n <= 0)
                return -ENODEV;
        for (; i < n; i++) {
-               cpu_node = of_parse_phandle(dev, "cpus", i);
+               cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
                if (!cpu_node)
                        break;
                cpu = of_cpu_node_to_id(cpu_node);
@@ -626,11 +630,51 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, 
cpumask_t *mask)
                 */
                if (cpu < 0)
                        continue;
-               cpumask_set_cpu(cpu, mask);
+               cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
        }
        return 0;
  }
  
+/**
+ * dsu_pmu_acpi_get_cpus: Get the list of CPUs in the cluster
+ * from ACPI.
+ */
+static int dsu_pmu_acpi_get_cpus(struct platform_device *pdev)
+{
+       int cpu;
+       struct dsu_pmu *dsu_pmu = (struct dsu_pmu *) platform_get_drvdata(pdev);
+
+       /*
+        * A dsu pmu node is inside a cluster parent node along with cpu nodes.
+        * We need to find out all cpus that have the same parent with this pmu.
+        */
+       for_each_possible_cpu(cpu) {
+               struct acpi_device *acpi_dev = 
ACPI_COMPANION(get_cpu_device(cpu));

minor nit: Please could we split this into :
                
                struct acpi_device *acpi_dev;
                struct device *cpu_dev = get_cpu_device(cpu);

                if (!cpu_dev)
                        continue;

                acpi_dev = ACPI_COMPANION(cpu_dev); ...

So that we are explicitly clear that we don't end up in NULL pointer 
dereferences (ACPI_COMPANION() is safe btw). Otherwise, with Will's
comments addressed, looks fine to me.

Appreciate a Cc in the next posting.

Suzuki