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

[Ilkka Koskinen]

Hi Robin,

I have a couple of comments below

On Thu, 1 Jun 2023, Robin Murphy wrote:
> 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 <robin.murphy@xxxxxxx>
> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
> drivers/perf/arm_cspmu/arm_cspmu.h |  4 +--
> 2 files changed, 13 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 3b91115c376d..f8daf252a488 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c

...

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

> 	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

> 	const struct impl_match *match = impl_match;
>
> 	/*

...

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

Ditto

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

...

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

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

	dev = cspmu->dev;

> -	apmt_node = cspmu->apmt_node;
> +	apmt_node = dev_get_platdata(dev);

Ditto

> 	affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;


Otherwise the patch looks good to me.

Cheers, Ilkka