lkml.org 
[lkml]   [2023]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

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@arm.com>
> ---
> 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

\
 
 \ /
  Last update: 2023-06-05 09:15    [W:0.110 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site