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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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