Messages in this thread | | | Date | Mon, 5 Jun 2023 11:49:32 +0100 | Subject | Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency | From | Robin Murphy <> |
| |
On 2023-06-05 08:12, Ilkka Koskinen wrote: > > 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);
Oof, indeed it looks like I got the wrong thing into my head at the critical moment :(
Clearly this deserves a nice helpful wrapper so we only have to think about the nasty casting once...
>> 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;
Good catch - attributing the error message to the wrong device doesn't matter too much currently, but this change does then need it to be right. Will fix that too.
Thanks! Robin.
> >> - 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
| |