| Message ID | 20230404134225.13408-1-Jonathan.Cameron@huawei.com |
|---|---|
| Headers | show |
| Series | Add parents to struct pmu -> dev | expand |
On Tue, Apr 04, 2023 at 02:41:53PM +0100, Jonathan Cameron wrote: > These are the low hanging fruit following GregKH's feedback that > all the devices registered via perf_pmu_register() should have parents. > > Note that this causes potential ABI breakage. > > It may fall in the category of it isn't breakage if no one notices > but I can't be certain of that. Whilst it is arguable that > no one should be been accessing PMUs except via the event_source > bus, there was documentation suggesting /sys/devices/ for particular > PMUs (because it was a shorter path?) devices can always move around /sys/devices/ as there is not a guarantee that they will ever be in the same place. That's what /sys/class/ is used to find (and /sys/bus/ in some cases.) And even then, the naming scheme is variable, and can and will change (i.e. bus ids), so that too is not required to stay the same. thanks for doing this work, I'll add it to my review queue... greg k-h
On Tue, Apr 04, 2023 at 02:41:54PM +0100, Jonathan Cameron wrote: > Some PMUs have well defined parents such as PCI devices. > As the device_initialize() and device_add() are all within > pmu_dev_alloc() which is called from perf_pmu_register() > there is no opportunity to set the parent from within a driver. > > Add a struct device *parent field to struct pmu and use that > to set the parent. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Apr 04, 2023 at 02:41:55PM +0100, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the PCI device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/hisilicon/hisi_pcie_pmu.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Apr 04, 2023 at 02:41:56PM +0100, Jonathan Cameron wrote: > Having assigned a parent to the device, the suggested path is > no longer valid. As /sys/bus/event_sources based path is also > provided, simply drop mention of alternative. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Apr 04, 2023 at 02:41:57PM +0100, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Apr 04, 2023 at 02:41:58PM +0100, Jonathan Cameron wrote: > To allow setting an appropriate parent for the struct pmu device > remove existing references to /sys/devices/ path. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Apr 04, 2023 at 02:41:53PM +0100, Jonathan Cameron wrote: > These are the low hanging fruit following GregKH's feedback that > all the devices registered via perf_pmu_register() should have parents. > > Note that this causes potential ABI breakage. > > It may fall in the category of it isn't breakage if no one notices > but I can't be certain of that. Whilst it is arguable that > no one should be been accessing PMUs except via the event_source > bus, there was documentation suggesting /sys/devices/ for particular > PMUs (because it was a shorter path?) > > The first patch is pulled out of the series: > https://lore.kernel.org/linux-cxl/20230327170247.6968-1-Jonathan.Cameron@huawei.com/ > [PATCH v3 0/5] CXL 3.0 Performance Monitoring Unit support > > In that particular case it is very useful to be able to figure out which > CXL device the PMU device is associated with and looking at it's parents > in the device model as shown with ls -lh /sys/bus/event_sources/devices/ > is a very easy way to do this (once it is correctly parented). > > Addressing all the other instances of struct pmu not covered by this series > is likely to be a more complex discussion but unlikely to have an affect > on what is proposed here. > > Documentation updates deliberately 'fixed' in separate patches before > changing the path to highlight that using /sys/bus/event_source/devices > path is unchanged by this series and that is presumed to be the > most common way these files are accessed. For the whole series, looks good: Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On 04/04/2023 14:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/perf/arm-cci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c > index 03b1309875ae..502581ba7d15 100644 > --- a/drivers/perf/arm-cci.c > +++ b/drivers/perf/arm-cci.c > @@ -1412,6 +1412,7 @@ static int cci_pmu_init(struct cci_pmu *cci_pmu, struct platform_device *pdev) > > cci_pmu->pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = &pdev->dev, > .name = cci_pmu->model->name, > .task_ctx_nr = perf_invalid_context, > .pmu_enable = cci_pmu_enable,
On 04/04/2023 14:42, Jonathan Cameron wrote: > Currently all these devices appear directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parents to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/arm_cspmu/arm_cspmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c > index e31302ab7e37..4390e4fb1e95 100644 > --- a/drivers/perf/arm_cspmu/arm_cspmu.c > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c > @@ -1155,6 +1155,7 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu) > cspmu->pmu = (struct pmu){ > .task_ctx_nr = perf_invalid_context, > .module = THIS_MODULE, > + .parent = cspmu->dev, > .pmu_enable = arm_cspmu_enable, > .pmu_disable = arm_cspmu_disable, > .event_init = arm_cspmu_event_init, Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On 04/04/2023 14:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/arm_dsu_pmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c > index fe2abb412c00..de75c00cb456 100644 > --- a/drivers/perf/arm_dsu_pmu.c > +++ b/drivers/perf/arm_dsu_pmu.c > @@ -751,6 +751,7 @@ static int dsu_pmu_device_probe(struct platform_device *pdev) > > dsu_pmu->pmu = (struct pmu) { > .task_ctx_nr = perf_invalid_context, > + .parent = &pdev->dev, > .module = THIS_MODULE, > .pmu_enable = dsu_pmu_enable, > .pmu_disable = dsu_pmu_disable, Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On 2023/4/4 PM9:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Cc: Shuai Xue <xueshuai@linux.alibaba.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/alibaba_uncore_drw_pmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/alibaba_uncore_drw_pmu.c b/drivers/perf/alibaba_uncore_drw_pmu.c > index a7689fecb49d..0d129acf3f84 100644 > --- a/drivers/perf/alibaba_uncore_drw_pmu.c > +++ b/drivers/perf/alibaba_uncore_drw_pmu.c > @@ -683,6 +683,7 @@ static int ali_drw_pmu_probe(struct platform_device *pdev) > > drw_pmu->pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = &pdev->dev, > .task_ctx_nr = perf_invalid_context, > .event_init = ali_drw_pmu_event_init, > .add = ali_drw_pmu_add, Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com> Thank you. Best Regards, Shuai
Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: nvdimm@lists.linux.dev > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On 2023/4/4 21:42, Jonathan Cameron wrote: > To allow for assigning a suitable parent to the struct pmu device > update the documentation to describe the device via the event_source > bus where it will remain accessible. > > For the ABI documention file also rename the file as it is named > after the path. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> > --- > ...i_ptt => sysfs-bus-event_source-devices-hisi_ptt} | 12 ++++++------ > Documentation/trace/hisi-ptt.rst | 4 ++-- > MAINTAINERS | 2 +- > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-hisi_ptt b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hisi_ptt > similarity index 83% > rename from Documentation/ABI/testing/sysfs-devices-hisi_ptt > rename to Documentation/ABI/testing/sysfs-bus-event_source-devices-hisi_ptt > index 82de6d710266..f2f48f7ce887 100644 > --- a/Documentation/ABI/testing/sysfs-devices-hisi_ptt > +++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hisi_ptt > @@ -1,4 +1,4 @@ > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > @@ -8,7 +8,7 @@ Description: This directory contains files for tuning the PCIe link > > See Documentation/trace/hisi-ptt.rst for more information. > > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_cpl > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_cpl > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > @@ -18,7 +18,7 @@ Description: (RW) Controls the weight of Tx completion TLPs, which influence > will return an error, and out of range values will be converted > to 2. The value indicates a probable level of the event. > > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_np > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_np > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > @@ -28,7 +28,7 @@ Description: (RW) Controls the weight of Tx non-posted TLPs, which influence > will return an error, and out of range values will be converted > to 2. The value indicates a probable level of the event. > > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_p > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune/qos_tx_p > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > @@ -38,7 +38,7 @@ Description: (RW) Controls the weight of Tx posted TLPs, which influence the > will return an error, and out of range values will be converted > to 2. The value indicates a probable level of the event. > > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune/rx_alloc_buf_level > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune/rx_alloc_buf_level > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > @@ -49,7 +49,7 @@ Description: (RW) Control the allocated buffer watermark for inbound packets. > will return an error, and out of range values will be converted > to 2. The value indicates a probable level of the event. > > -What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune/tx_alloc_buf_level > +What: /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune/tx_alloc_buf_level > Date: October 2022 > KernelVersion: 6.1 > Contact: Yicong Yang <yangyicong@hisilicon.com> > diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst > index 4f87d8e21065..d923e09fcbaa 100644 > --- a/Documentation/trace/hisi-ptt.rst > +++ b/Documentation/trace/hisi-ptt.rst > @@ -40,7 +40,7 @@ IO dies (SICL, Super I/O Cluster), where there's one PCIe Root > Complex for each SICL. > :: > > - /sys/devices/hisi_ptt<sicl_id>_<core_id> > + /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id> > > Tune > ==== > @@ -53,7 +53,7 @@ Each event is presented as a file under $(PTT PMU dir)/tune, and > a simple open/read/write/close cycle will be used to tune the event. > :: > > - $ cd /sys/devices/hisi_ptt<sicl_id>_<core_id>/tune > + $ cd /sys/bus/event_source/devices/hisi_ptt<sicl_id>_<core_id>/tune > $ ls > qos_tx_cpl qos_tx_np qos_tx_p > tx_path_rx_req_alloc_buf_level > diff --git a/MAINTAINERS b/MAINTAINERS > index d8ebab595b2a..75019f62b1df 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9266,7 +9266,7 @@ M: Yicong Yang <yangyicong@hisilicon.com> > M: Jonathan Cameron <jonathan.cameron@huawei.com> > L: linux-kernel@vger.kernel.org > S: Maintained > -F: Documentation/ABI/testing/sysfs-devices-hisi_ptt > +F: Documentation/ABI/testing/sysfs-bus-event_source-devices-hisi_ptt > F: Documentation/trace/hisi-ptt.rst > F: drivers/hwtracing/ptt/ > F: tools/perf/arch/arm64/util/hisi-ptt.c >
On 2023/4/4 21:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the PCI device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Cc: Yicong Yang <yangyicong@hisilicon.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/hwtracing/ptt/hisi_ptt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c > index 30f1525639b5..3868d43e9e3c 100644 > --- a/drivers/hwtracing/ptt/hisi_ptt.c > +++ b/drivers/hwtracing/ptt/hisi_ptt.c > @@ -871,6 +871,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt) > > hisi_ptt->hisi_ptt_pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = &hisi_ptt->pdev->dev, > .capabilities = PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE, > .task_ctx_nr = perf_sw_context, > .attr_groups = hisi_ptt_pmu_groups, >
On 2023/4/4 21:41, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the PCI device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/perf/hisilicon/hisi_pcie_pmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c > index 6fee0b6e163b..2cc88d75b895 100644 > --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c > +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c > @@ -793,6 +793,7 @@ static int hisi_pcie_alloc_pmu(struct pci_dev *pdev, struct hisi_pcie_pmu *pcie_ > pcie_pmu->pmu = (struct pmu) { > .name = name, > .module = THIS_MODULE, > + .parent = &pdev->dev, > .event_init = hisi_pcie_pmu_event_init, > .pmu_enable = hisi_pcie_pmu_enable, > .pmu_disable = hisi_pcie_pmu_disable, >
On 2023/4/4 21:41, Jonathan Cameron wrote: > Having assigned a parent to the device, the suggested path is > no longer valid. As /sys/bus/event_sources based path is also > provided, simply drop mention of alternative. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> > --- > Documentation/admin-guide/perf/hisi-pmu.rst | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/Documentation/admin-guide/perf/hisi-pmu.rst b/Documentation/admin-guide/perf/hisi-pmu.rst > index 546979360513..1ddab80769d3 100644 > --- a/Documentation/admin-guide/perf/hisi-pmu.rst > +++ b/Documentation/admin-guide/perf/hisi-pmu.rst > @@ -20,7 +20,6 @@ interrupt, and the PMU driver shall register perf PMU drivers like L3C, > HHA and DDRC etc. The available events and configuration options shall > be described in the sysfs, see: > > -/sys/devices/hisi_sccl{X}_<l3c{Y}/hha{Y}/ddrc{Y}>/, or > /sys/bus/event_source/devices/hisi_sccl{X}_<l3c{Y}/hha{Y}/ddrc{Y}>. > The "perf list" command shall list the available events from sysfs. > >
On 2023/4/4 21:41, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/perf/hisilicon/hisi_uncore_pmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c > index f1b0f5e1a28f..b4350e5dc3fc 100644 > --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c > +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c > @@ -538,6 +538,7 @@ void hisi_pmu_init(struct hisi_pmu *hisi_pmu, const char *name, > > pmu->name = name; > pmu->module = module; > + pmu->parent = hisi_pmu->dev; > pmu->task_ctx_nr = perf_invalid_context; > pmu->event_init = hisi_uncore_pmu_event_init; > pmu->pmu_enable = hisi_uncore_pmu_enable; >
On 2023/4/4 21:41, Jonathan Cameron wrote: > Some PMUs have well defined parents such as PCI devices. > As the device_initialize() and device_add() are all within > pmu_dev_alloc() which is called from perf_pmu_register() > there is no opportunity to set the parent from within a driver. > > Add a struct device *parent field to struct pmu and use that > to set the parent. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > --- > Previously posted in CPMU series hence the change log. > v3: No change > --- > include/linux/perf_event.h | 1 + > kernel/events/core.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index d5628a7b5eaa..b99db1eda72c 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -303,6 +303,7 @@ struct pmu { > > struct module *module; > struct device *dev; > + struct device *parent; > const struct attribute_group **attr_groups; > const struct attribute_group **attr_update; > const char *name; > diff --git a/kernel/events/core.c b/kernel/events/core.c > index fb3e436bcd4a..a84c282221f2 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -11367,6 +11367,7 @@ static int pmu_dev_alloc(struct pmu *pmu) > > dev_set_drvdata(pmu->dev, pmu); > pmu->dev->bus = &pmu_bus; > + pmu->dev->parent = pmu->parent; If there's no parent assigned, is it ok to add some check here? Then we can find it earlier maybe at develop stage. Thanks. > pmu->dev->release = pmu_dev_release; > > ret = dev_set_name(pmu->dev, "%s", pmu->name); >
On Thu, 6 Apr 2023 12:03:27 +0800 Yicong Yang <yangyicong@huawei.com> wrote: > On 2023/4/4 21:41, Jonathan Cameron wrote: > > Some PMUs have well defined parents such as PCI devices. > > As the device_initialize() and device_add() are all within > > pmu_dev_alloc() which is called from perf_pmu_register() > > there is no opportunity to set the parent from within a driver. > > > > Add a struct device *parent field to struct pmu and use that > > to set the parent. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > --- > > Previously posted in CPMU series hence the change log. > > v3: No change > > --- > > include/linux/perf_event.h | 1 + > > kernel/events/core.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index d5628a7b5eaa..b99db1eda72c 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -303,6 +303,7 @@ struct pmu { > > > > struct module *module; > > struct device *dev; > > + struct device *parent; > > const struct attribute_group **attr_groups; > > const struct attribute_group **attr_update; > > const char *name; > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index fb3e436bcd4a..a84c282221f2 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -11367,6 +11367,7 @@ static int pmu_dev_alloc(struct pmu *pmu) > > > > dev_set_drvdata(pmu->dev, pmu); > > pmu->dev->bus = &pmu_bus; > > + pmu->dev->parent = pmu->parent; > > If there's no parent assigned, is it ok to add some check here? Then we can find it earlier > maybe at develop stage. In the long run I agree it would be good. Short term there are more instances of struct pmu that don't have parents than those that do (even after this series). We need to figure out what to do about those before adding checks on it being set. Jonathan > > Thanks. > > > pmu->dev->release = pmu_dev_release; > > > > ret = dev_set_name(pmu->dev, "%s", pmu->name); > >
On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: > In the long run I agree it would be good. Short term there are more instances of > struct pmu that don't have parents than those that do (even after this series). > We need to figure out what to do about those before adding checks on it being > set. Right, I don't think you've touched *any* of the x86 PMUs for example, and getting everybody that boots an x86 kernel a warning isn't going to go over well :-)
On Thu, 6 Apr 2023 14:40:40 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: > > > In the long run I agree it would be good. Short term there are more instances of > > struct pmu that don't have parents than those that do (even after this series). > > We need to figure out what to do about those before adding checks on it being > > set. > > Right, I don't think you've touched *any* of the x86 PMUs for example, > and getting everybody that boots an x86 kernel a warning isn't going to > go over well :-) > It was tempting :) "Warning: Parentless PMU: try a different architecture." I'd love some inputs on what the x86 PMU devices parents should be? CPU counters in general tend to just spin out of deep in the architecture code. My overall favorite is an l2 cache related PMU that is spun up in arch/arm/kernel/irq.c init_IRQ() I'm just not going to try and figure out why... Jonathan
On Thu, Apr 06, 2023 at 05:44:45PM +0100, Jonathan Cameron wrote: > On Thu, 6 Apr 2023 14:40:40 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: > > > > > In the long run I agree it would be good. Short term there are more instances of > > > struct pmu that don't have parents than those that do (even after this series). > > > We need to figure out what to do about those before adding checks on it being > > > set. > > > > Right, I don't think you've touched *any* of the x86 PMUs for example, > > and getting everybody that boots an x86 kernel a warning isn't going to > > go over well :-) > > > > It was tempting :) "Warning: Parentless PMU: try a different architecture." > > I'd love some inputs on what the x86 PMU devices parents should be? > CPU counters in general tend to just spin out of deep in the architecture code. > > My overall favorite is an l2 cache related PMU that is spun up in > arch/arm/kernel/irq.c init_IRQ() > > I'm just not going to try and figure out why... Why not change the api to force a parent to be passed in? And if one isn't, we make it a "virtual" device and throw it in the class for them? thanks, greg k-h
On Thu, Apr 06, 2023 at 05:44:45PM +0100, Jonathan Cameron wrote: > On Thu, 6 Apr 2023 14:40:40 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: > > > > > In the long run I agree it would be good. Short term there are more instances of > > > struct pmu that don't have parents than those that do (even after this series). > > > We need to figure out what to do about those before adding checks on it being > > > set. > > > > Right, I don't think you've touched *any* of the x86 PMUs for example, > > and getting everybody that boots an x86 kernel a warning isn't going to > > go over well :-) > > > > It was tempting :) "Warning: Parentless PMU: try a different architecture." Haha! > I'd love some inputs on what the x86 PMU devices parents should be? > CPU counters in general tend to just spin out of deep in the architecture code. For the 'simple' ones I suppose we can use the CPU device. > My overall favorite is an l2 cache related PMU that is spun up in > arch/arm/kernel/irq.c init_IRQ() Yeah, we're going to have a ton of them as well. Some of them are PCI devices and have a clear parent, others, not so much :/
On 2023-04-04 at 14:42:22 +0100, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the Platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Cc: Wu Hao <hao.wu@intel.com> > Cc: Tom Rix <trix@redhat.com> > Cc: linux-fpga@vger.kernel.org > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Xu Yilun <yilun.xu@intel.com> > --- > drivers/fpga/dfl-fme-perf.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/fpga/dfl-fme-perf.c b/drivers/fpga/dfl-fme-perf.c > index 7422d2bc6f37..2d59f1c620b1 100644 > --- a/drivers/fpga/dfl-fme-perf.c > +++ b/drivers/fpga/dfl-fme-perf.c > @@ -912,6 +912,7 @@ static int fme_perf_pmu_register(struct platform_device *pdev, > > fme_perf_setup_hardware(priv); > > + pmu->parent = &pdev->dev; > pmu->task_ctx_nr = perf_invalid_context; > pmu->attr_groups = fme_perf_groups; > pmu->attr_update = fme_perf_events_groups; > -- > 2.37.2 >
On 2023/4/4 21:42, Jonathan Cameron wrote: > [ EXTERNAL EMAIL ] > > Currently all these devices appear directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parents to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Cc: Jiucheng Xu <jiucheng.xu@amlogic.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/amlogic/meson_ddr_pmu_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/amlogic/meson_ddr_pmu_core.c b/drivers/perf/amlogic/meson_ddr_pmu_core.c > index b84346dbac2c..e0d3e87457e0 100644 > --- a/drivers/perf/amlogic/meson_ddr_pmu_core.c > +++ b/drivers/perf/amlogic/meson_ddr_pmu_core.c > @@ -490,6 +490,7 @@ int meson_ddr_pmu_create(struct platform_device *pdev) > *pmu = (struct ddr_pmu) { > .pmu = { > .module = THIS_MODULE, > + .parent = &pdev->dev, Reviewed-by: Jiucheng Xu <jiucheng.xu@amlogic.com > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > .task_ctx_nr = perf_invalid_context, > .attr_groups = attr_groups,
On Thu, 6 Apr 2023 19:08:45 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 06, 2023 at 05:44:45PM +0100, Jonathan Cameron wrote: > > On Thu, 6 Apr 2023 14:40:40 +0200 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: > > > > > > > In the long run I agree it would be good. Short term there are more instances of > > > > struct pmu that don't have parents than those that do (even after this series). > > > > We need to figure out what to do about those before adding checks on it being > > > > set. > > > > > > Right, I don't think you've touched *any* of the x86 PMUs for example, > > > and getting everybody that boots an x86 kernel a warning isn't going to > > > go over well :-) > > > > > > > It was tempting :) "Warning: Parentless PMU: try a different architecture." > > > > I'd love some inputs on what the x86 PMU devices parents should be? > > CPU counters in general tend to just spin out of deep in the architecture code. > > > > My overall favorite is an l2 cache related PMU that is spun up in > > arch/arm/kernel/irq.c init_IRQ() > > > > I'm just not going to try and figure out why... > > Why not change the api to force a parent to be passed in? And if one > isn't, we make it a "virtual" device and throw it in the class for them? Longer term I'd be fine doing that, but I'd like to identify the right parents rather than end up sweeping it under the carpet. Anything we either get completely stuck on (or decide we don't care about) could indeed fall back to a virtual device. Jonathan > > thanks, > > greg k-h
On 2023-04-04 14:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. Oh, fab! Just the other week I wrote up a patch doing this after the fact with device_move() on the grounds of making the PMU instances easier to identify (and with an equivalent cleanup of Documentation/admin-guide/perf), which I was close to getting round to sending as an RFC. Thus I thoroughly approve :D Acked-by: Robin Murphy <robin.murphy@arm.com> > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/arm-cmn.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index c9689861be3f..7731eb0e2a4a 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -2284,6 +2284,7 @@ static int arm_cmn_probe(struct platform_device *pdev) > cmn->cpu = cpumask_local_spread(0, dev_to_node(cmn->dev)); > cmn->pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = &pdev->dev, > .attr_groups = arm_cmn_attr_groups, > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > .task_ctx_nr = perf_invalid_context,
On 04/04/2023 14:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/arm_spe_pmu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index b9ba4c4fe5a2..a98ef633fa00 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -955,6 +955,7 @@ static int arm_spe_pmu_perf_init(struct arm_spe_pmu *spe_pmu) > > spe_pmu->pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = &spe_pmu->pdev->dev, > .capabilities = PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE, > .attr_groups = arm_spe_pmu_attr_groups, > /* Acked-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On 04/04/2023 14:42, Jonathan Cameron wrote: > Currently the PMU device appears directly under /sys/devices/ > Only root busses should appear there, so instead assign the pmu->dev > parent to be the platform device. > > Link: https://lore.kernel.org/linux-cxl/ZCLI9A40PJsyqAmq@kroah.com/ > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/perf/arm-ccn.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c > index 728d13d8e98a..dc8b0dcb436e 100644 > --- a/drivers/perf/arm-ccn.c > +++ b/drivers/perf/arm-ccn.c > @@ -1265,6 +1265,7 @@ static int arm_ccn_pmu_init(struct arm_ccn *ccn) > /* Perf driver registration */ > ccn->dt.pmu = (struct pmu) { > .module = THIS_MODULE, > + .parent = ccn->dev, > .attr_groups = arm_ccn_pmu_attr_groups, > .task_ctx_nr = perf_invalid_context, > .event_init = arm_ccn_pmu_event_init, Acked-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On 2023-04-06 17:44, Jonathan Cameron wrote: > On Thu, 6 Apr 2023 14:40:40 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > >> On Thu, Apr 06, 2023 at 11:16:07AM +0100, Jonathan Cameron wrote: >> >>> In the long run I agree it would be good. Short term there are more instances of >>> struct pmu that don't have parents than those that do (even after this series). >>> We need to figure out what to do about those before adding checks on it being >>> set. >> >> Right, I don't think you've touched *any* of the x86 PMUs for example, >> and getting everybody that boots an x86 kernel a warning isn't going to >> go over well :-) >> > > It was tempting :) "Warning: Parentless PMU: try a different architecture." > > I'd love some inputs on what the x86 PMU devices parents should be? > CPU counters in general tend to just spin out of deep in the architecture code. > > My overall favorite is an l2 cache related PMU that is spun up in > arch/arm/kernel/irq.c init_IRQ() > > I'm just not going to try and figure out why... I think that's simply because the PMU support was hung off the existing PL310 configuration code, which still supports non-DT boardfiles. The PMU shouldn't strictly need to be registered that early, it would just be a bunch more work to ensure that a platform device is available for it to bind to as a regular driver model driver, which wasn't justifiable at the time. Thanks, Robin.
These are the low hanging fruit following GregKH's feedback that all the devices registered via perf_pmu_register() should have parents. Note that this causes potential ABI breakage. It may fall in the category of it isn't breakage if no one notices but I can't be certain of that. Whilst it is arguable that no one should be been accessing PMUs except via the event_source bus, there was documentation suggesting /sys/devices/ for particular PMUs (because it was a shorter path?) The first patch is pulled out of the series: https://lore.kernel.org/linux-cxl/20230327170247.6968-1-Jonathan.Cameron@huawei.com/ [PATCH v3 0/5] CXL 3.0 Performance Monitoring Unit support In that particular case it is very useful to be able to figure out which CXL device the PMU device is associated with and looking at it's parents in the device model as shown with ls -lh /sys/bus/event_sources/devices/ is a very easy way to do this (once it is correctly parented). Addressing all the other instances of struct pmu not covered by this series is likely to be a more complex discussion but unlikely to have an affect on what is proposed here. Documentation updates deliberately 'fixed' in separate patches before changing the path to highlight that using /sys/bus/event_source/devices path is unchanged by this series and that is presumed to be the most common way these files are accessed. Jonathan Cameron (32): perf: Allow a PMU to have a parent perf/hisi-pcie: Assign parent for event_source device Documentation: hisi-pmu: Drop reference to /sys/devices path perf/hisi-uncore: Assign parents for event_source devices Documentation: hns-pmu: Use /sys/bus/event_source/devices paths perf/hisi-hns3: Assign parents for event_source device perf/amlogic: Assign parents for event_source devices perf/arm_cspmu: Assign parents for event_source devices Documentation: xgene-pmu: Use /sys/bus/event_source/devices paths perf/xgene: Assign parents for event_source devices Documentation: thunderx2-pmu: Use /sys/bus/event_source/devices paths perf/thunderx2: Assign parents for event_source devices perf/riscv: Assign parents for event_source devices Documentation: qcom-pmu: Use /sys/bus/event_source/devices paths perf/qcom: Assign parents for event_source devices perf/imx_ddr: Assign parents for event_source devices perf/arm_pmu: Assign parents for event_source devices perf/alibaba_uncore: Assign parents for event_source device perf/arm-cci: Assign parents for event_source device perf/arm-ccn: Assign parents for event_source device perf/arm-cmn: Assign parents for event_source device perf/arm-dmc620: Assign parents for event_source device perf/arm-dsu: Assign parents for event_source device perf/arm-smmuv3: Assign parents for event_source device perf/arm-spe: Assign parents for event_source device arc: Assign parents for event_source devices ARM: imx: Assign parents for mmdc event_source devices dmaengine: idxd: Assign parent for event_source device fpga: dfl: Assign parent for event_source device drivers/nvdimm: Assign parent for event_source device Documentation: ABI + trace: hisi_ptt: update paths to bus/event_source hwtracing: hisi_ptt: Assign parent for event_source device ...i_ptt => sysfs-bus-event_source-devices-hisi_ptt} | 12 ++++++------ Documentation/admin-guide/perf/hisi-pmu.rst | 1 - Documentation/admin-guide/perf/hns3-pmu.rst | 8 ++++---- Documentation/admin-guide/perf/qcom_l2_pmu.rst | 2 +- Documentation/admin-guide/perf/qcom_l3_pmu.rst | 2 +- Documentation/admin-guide/perf/thunderx2-pmu.rst | 2 +- Documentation/admin-guide/perf/xgene-pmu.rst | 2 +- Documentation/trace/hisi-ptt.rst | 4 ++-- MAINTAINERS | 2 +- arch/arc/kernel/perf_event.c | 1 + arch/arm/mach-imx/mmdc.c | 1 + drivers/dma/idxd/perfmon.c | 1 + drivers/fpga/dfl-fme-perf.c | 1 + drivers/hwtracing/ptt/hisi_ptt.c | 1 + drivers/nvdimm/nd_perf.c | 1 + drivers/perf/alibaba_uncore_drw_pmu.c | 1 + drivers/perf/amlogic/meson_ddr_pmu_core.c | 1 + drivers/perf/arm-cci.c | 1 + drivers/perf/arm-ccn.c | 1 + drivers/perf/arm-cmn.c | 1 + drivers/perf/arm_cspmu/arm_cspmu.c | 1 + drivers/perf/arm_dmc620_pmu.c | 1 + drivers/perf/arm_dsu_pmu.c | 1 + drivers/perf/arm_pmu_platform.c | 1 + drivers/perf/arm_smmuv3_pmu.c | 1 + drivers/perf/arm_spe_pmu.c | 1 + drivers/perf/fsl_imx8_ddr_perf.c | 1 + drivers/perf/hisilicon/hisi_pcie_pmu.c | 1 + drivers/perf/hisilicon/hisi_uncore_pmu.c | 1 + drivers/perf/hisilicon/hns3_pmu.c | 1 + drivers/perf/qcom_l2_pmu.c | 1 + drivers/perf/qcom_l3_pmu.c | 1 + drivers/perf/riscv_pmu_legacy.c | 1 + drivers/perf/riscv_pmu_sbi.c | 1 + drivers/perf/thunderx2_pmu.c | 1 + drivers/perf/xgene_pmu.c | 1 + include/linux/perf_event.h | 1 + kernel/events/core.c | 1 + 38 files changed, 46 insertions(+), 18 deletions(-) rename Documentation/ABI/testing/{sysfs-devices-hisi_ptt => sysfs-bus-event_source-devices-hisi_ptt} (83%)