| Message ID | 6858523689a214543224495fee22f9e31be2f767.1702571292.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/arm_cspmu: Add devicetree support | expand |
On Thu, 14 Dec 2023, Robin Murphy wrote: > Hook up devicetree probing support. For now let's hope that people > implement PMIIDR properly and we don't need an override property or > match data mechanism. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> Thanks, this version works fine with ACPI. Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Cheers, Ilkka > --- > v2: Use APMT node to distinguish ACPI; adjust for binding change > --- > drivers/perf/arm_cspmu/arm_cspmu.c | 63 ++++++++++++++++++++++++------ > 1 file changed, 52 insertions(+), 11 deletions(-) > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c > index b64de4d800c7..6c76c135a0cf 100644 > --- a/drivers/perf/arm_cspmu/arm_cspmu.c > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c > @@ -27,6 +27,7 @@ > #include <linux/io-64-nonatomic-lo-hi.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of.h> > #include <linux/perf_event.h> > #include <linux/platform_device.h> > > @@ -310,6 +311,10 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu) > > dev = cspmu->dev; > apmt_node = arm_cspmu_apmt_node(dev); > + if (!apmt_node) > + return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u", > + atomic_fetch_inc(&pmu_idx[0])); > + > pmu_type = apmt_node->type; > > if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) { > @@ -425,7 +430,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu) > }; > > /* Firmware may override implementer/product ID from PMIIDR */ > - if (apmt_node->impl_id) > + if (apmt_node && apmt_node->impl_id) > cspmu->impl.pmiidr = apmt_node->impl_id; > > /* Find implementer specific attribute ops. */ > @@ -940,7 +945,14 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev) > platform_set_drvdata(pdev, cspmu); > > apmt_node = arm_cspmu_apmt_node(dev); > - cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC; > + if (apmt_node) { > + cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC; > + } else { > + u32 width = 0; > + > + device_property_read_u32(dev, "reg-io-width", &width); > + cspmu->has_atomic_dword = (width == 8); > + } > > return cspmu; > } > @@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) > } > } > > - if (cpumask_empty(&cspmu->associated_cpus)) { > - dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); > - return -ENODEV; > - } > - > return 0; > } > #else > @@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) > } > #endif > > +static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu) > +{ > + struct of_phandle_iterator it; > + int ret, cpu; > + > + of_for_each_phandle(&it, ret, dev_of_node(cspmu->dev), "cpus", NULL, 0) { > + cpu = of_cpu_node_to_id(it.node); > + if (cpu < 0) > + continue; > + cpumask_set_cpu(cpu, &cspmu->associated_cpus); > + } > + return ret; > +} > + > static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu) > { > - return arm_cspmu_acpi_get_cpus(cspmu); > + int ret = 0; > + > + if (arm_cspmu_apmt_node(cspmu->dev)) > + ret = arm_cspmu_acpi_get_cpus(cspmu); > + else if (device_property_present(cspmu->dev, "cpus")) > + ret = arm_cspmu_of_get_cpus(cspmu); > + else > + cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask); > + > + if (!ret && cpumask_empty(&cspmu->associated_cpus)) { > + dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); > + ret = -ENODEV; > + } > + return ret; > } > > static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu) > @@ -1246,11 +1280,18 @@ static const struct platform_device_id arm_cspmu_id[] = { > }; > MODULE_DEVICE_TABLE(platform, arm_cspmu_id); > > +static const struct of_device_id arm_cspmu_of_match[] = { > + { .compatible = "arm,coresight-pmu" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, arm_cspmu_of_match); > + > static struct platform_driver arm_cspmu_driver = { > .driver = { > - .name = DRVNAME, > - .suppress_bind_attrs = true, > - }, > + .name = DRVNAME, > + .of_match_table = arm_cspmu_of_match, > + .suppress_bind_attrs = true, > + }, > .probe = arm_cspmu_device_probe, > .remove = arm_cspmu_device_remove, > .id_table = arm_cspmu_id, > -- > 2.39.2.101.g768bb238c484.dirty > >
Hi Robin, Please see my comment inline. > -----Original Message----- > From: Robin Murphy <robin.murphy@arm.com> > Sent: Thursday, December 14, 2023 10:31 AM > To: will@kernel.org > Cc: mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; suzuki.poulose@arm.com; > ilkka@os.amperecomputing.com; Besar Wicaksono > <bwicaksono@nvidia.com>; Yifei Wan <YWan@nvidia.com>; Richard Wiley > <rwiley@nvidia.com> > Subject: [PATCH v2 5/5] perf/arm_cspmu: Add devicetree support > > External email: Use caution opening links or attachments > > > Hook up devicetree probing support. For now let's hope that people > implement PMIIDR properly and we don't need an override property or > match data mechanism. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > v2: Use APMT node to distinguish ACPI; adjust for binding change > --- > drivers/perf/arm_cspmu/arm_cspmu.c | 63 ++++++++++++++++++++++++- > ----- > 1 file changed, 52 insertions(+), 11 deletions(-) > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c > b/drivers/perf/arm_cspmu/arm_cspmu.c > index b64de4d800c7..6c76c135a0cf 100644 > --- a/drivers/perf/arm_cspmu/arm_cspmu.c > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c > @@ -27,6 +27,7 @@ > #include <linux/io-64-nonatomic-lo-hi.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of.h> > #include <linux/perf_event.h> > #include <linux/platform_device.h> > > @@ -310,6 +311,10 @@ static const char *arm_cspmu_get_name(const > struct arm_cspmu *cspmu) > > dev = cspmu->dev; > apmt_node = arm_cspmu_apmt_node(dev); > + if (!apmt_node) I got a segmentation fault due to null pointer access when calling arm_cspmu_apmt_node in device tree environment. arm_cspmu_apmt_node doesn't check a null platform data before dereferencing. Snipet from arm_cspmu_apmt_node: return *(struct acpi_apmt_node **)dev_get_platdata(dev); > + return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u", > + atomic_fetch_inc(&pmu_idx[0])); > + > pmu_type = apmt_node->type; > > if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) { > @@ -425,7 +430,7 @@ static int arm_cspmu_init_impl_ops(struct > arm_cspmu *cspmu) > }; > > /* Firmware may override implementer/product ID from PMIIDR */ > - if (apmt_node->impl_id) > + if (apmt_node && apmt_node->impl_id) > cspmu->impl.pmiidr = apmt_node->impl_id; > > /* Find implementer specific attribute ops. */ > @@ -940,7 +945,14 @@ static struct arm_cspmu *arm_cspmu_alloc(struct > platform_device *pdev) > platform_set_drvdata(pdev, cspmu); > > apmt_node = arm_cspmu_apmt_node(dev); > - cspmu->has_atomic_dword = apmt_node->flags & > ACPI_APMT_FLAGS_ATOMIC; > + if (apmt_node) { > + cspmu->has_atomic_dword = apmt_node->flags & > ACPI_APMT_FLAGS_ATOMIC; > + } else { > + u32 width = 0; > + > + device_property_read_u32(dev, "reg-io-width", &width); > + cspmu->has_atomic_dword = (width == 8); > + } > > return cspmu; > } > @@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct > arm_cspmu *cspmu) > } > } > > - if (cpumask_empty(&cspmu->associated_cpus)) { > - dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); > - return -ENODEV; > - } > - > return 0; > } > #else > @@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct > arm_cspmu *cspmu) > } > #endif > > +static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu) > +{ > + struct of_phandle_iterator it; > + int ret, cpu; > + > + of_for_each_phandle(&it, ret, dev_of_node(cspmu->dev), "cpus", NULL, > 0) { > + cpu = of_cpu_node_to_id(it.node); > + if (cpu < 0) > + continue; > + cpumask_set_cpu(cpu, &cspmu->associated_cpus); > + } > + return ret; The ret gives -ENOENT after finish iterating "cpus". I think we could return 0 or void since there is a check for empty associated_cpus down the line. Thanks, Besar > +} > + > static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu) > { > - return arm_cspmu_acpi_get_cpus(cspmu); > + int ret = 0; > + > + if (arm_cspmu_apmt_node(cspmu->dev)) > + ret = arm_cspmu_acpi_get_cpus(cspmu); > + else if (device_property_present(cspmu->dev, "cpus")) > + ret = arm_cspmu_of_get_cpus(cspmu); > + else > + cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask); > + > + if (!ret && cpumask_empty(&cspmu->associated_cpus)) { > + dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); > + ret = -ENODEV; > + } > + return ret; > } > > static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu) > @@ -1246,11 +1280,18 @@ static const struct platform_device_id > arm_cspmu_id[] = { > }; > MODULE_DEVICE_TABLE(platform, arm_cspmu_id); > > +static const struct of_device_id arm_cspmu_of_match[] = { > + { .compatible = "arm,coresight-pmu" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, arm_cspmu_of_match); > + > static struct platform_driver arm_cspmu_driver = { > .driver = { > - .name = DRVNAME, > - .suppress_bind_attrs = true, > - }, > + .name = DRVNAME, > + .of_match_table = arm_cspmu_of_match, > + .suppress_bind_attrs = true, > + }, > .probe = arm_cspmu_device_probe, > .remove = arm_cspmu_device_remove, > .id_table = arm_cspmu_id, > -- > 2.39.2.101.g768bb238c484.dirty
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c index b64de4d800c7..6c76c135a0cf 100644 --- a/drivers/perf/arm_cspmu/arm_cspmu.c +++ b/drivers/perf/arm_cspmu/arm_cspmu.c @@ -27,6 +27,7 @@ #include <linux/io-64-nonatomic-lo-hi.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of.h> #include <linux/perf_event.h> #include <linux/platform_device.h> @@ -310,6 +311,10 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu) dev = cspmu->dev; apmt_node = arm_cspmu_apmt_node(dev); + if (!apmt_node) + return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u", + atomic_fetch_inc(&pmu_idx[0])); + pmu_type = apmt_node->type; if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) { @@ -425,7 +430,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu) }; /* Firmware may override implementer/product ID from PMIIDR */ - if (apmt_node->impl_id) + if (apmt_node && apmt_node->impl_id) cspmu->impl.pmiidr = apmt_node->impl_id; /* Find implementer specific attribute ops. */ @@ -940,7 +945,14 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev) platform_set_drvdata(pdev, cspmu); apmt_node = arm_cspmu_apmt_node(dev); - cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC; + if (apmt_node) { + cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC; + } else { + u32 width = 0; + + device_property_read_u32(dev, "reg-io-width", &width); + cspmu->has_atomic_dword = (width == 8); + } return cspmu; } @@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) } } - if (cpumask_empty(&cspmu->associated_cpus)) { - dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); - return -ENODEV; - } - return 0; } #else @@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu) } #endif +static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu) +{ + struct of_phandle_iterator it; + int ret, cpu; + + of_for_each_phandle(&it, ret, dev_of_node(cspmu->dev), "cpus", NULL, 0) { + cpu = of_cpu_node_to_id(it.node); + if (cpu < 0) + continue; + cpumask_set_cpu(cpu, &cspmu->associated_cpus); + } + return ret; +} + static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu) { - return arm_cspmu_acpi_get_cpus(cspmu); + int ret = 0; + + if (arm_cspmu_apmt_node(cspmu->dev)) + ret = arm_cspmu_acpi_get_cpus(cspmu); + else if (device_property_present(cspmu->dev, "cpus")) + ret = arm_cspmu_of_get_cpus(cspmu); + else + cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask); + + if (!ret && cpumask_empty(&cspmu->associated_cpus)) { + dev_dbg(cspmu->dev, "No cpu associated with the PMU\n"); + ret = -ENODEV; + } + return ret; } static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu) @@ -1246,11 +1280,18 @@ static const struct platform_device_id arm_cspmu_id[] = { }; MODULE_DEVICE_TABLE(platform, arm_cspmu_id); +static const struct of_device_id arm_cspmu_of_match[] = { + { .compatible = "arm,coresight-pmu" }, + {} +}; +MODULE_DEVICE_TABLE(of, arm_cspmu_of_match); + static struct platform_driver arm_cspmu_driver = { .driver = { - .name = DRVNAME, - .suppress_bind_attrs = true, - }, + .name = DRVNAME, + .of_match_table = arm_cspmu_of_match, + .suppress_bind_attrs = true, + }, .probe = arm_cspmu_device_probe, .remove = arm_cspmu_device_remove, .id_table = arm_cspmu_id,
Hook up devicetree probing support. For now let's hope that people implement PMIIDR properly and we don't need an override property or match data mechanism. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- v2: Use APMT node to distinguish ACPI; adjust for binding change --- drivers/perf/arm_cspmu/arm_cspmu.c | 63 ++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 11 deletions(-)