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 <tuanphan@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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