| Message ID | 20230324171313.18448-3-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL 3.0 Performance Monitoring Unit support | expand |
On 2023-03-24 1:13 p.m., 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. Why we want a PMU parent? Maybe I missed it. I don't see that the parent is used anywhere. Thanks, Kan > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > 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; > pmu->dev->release = pmu_dev_release; > > ret = dev_set_name(pmu->dev, "%s", pmu->name);
On Mon, 27 Mar 2023 13:04:08 -0400 "Liang, Kan" <kan.liang@linux.intel.com> wrote: > On 2023-03-24 1:13 p.m., 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. > > Why we want a PMU parent? Maybe I missed it. I don't see that the parent > is used anywhere. This allows you to identify the association between PMU and the hardware related device that is providing it by looking at the directory structure in sysfs rather than putting them directly under /sys/devices. ls -l /sys/bus/event_sources/devices/ ... armv8_pmuv3_0 -> ../../../devices/arm8_pmuv3_0 ... breakpoint -> ../../../devices/breakpoint ... cpmu0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/cpmu0/cpmu0 etc (the first cpmu0 is the parent registered as a child of the PCI EP and used for driver binding). So it's of use to userspace rather than in the kernel driver itself. Note that almost nothing is normally in the top level /sys/devices other than event_sources - because nearly all other struct device instances created by subsystems have parents assigned. On my system ls /sys/devices armv8_pmuv3_0 LNXSYSTEM:00 pci0000:0c pnp0 system uprobe breakpoint pci0000:00 platform software tracepoint virtual +CC Greg KH for input on whether / why this make sense. Thanks, Jonathan > > Thanks, > Kan > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > 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; > > pmu->dev->release = pmu_dev_release; > > > > ret = dev_set_name(pmu->dev, "%s", pmu->name); >
On Tue, Mar 28, 2023 at 11:54:44AM +0100, Jonathan Cameron wrote: > On Mon, 27 Mar 2023 13:04:08 -0400 > "Liang, Kan" <kan.liang@linux.intel.com> wrote: > > > On 2023-03-24 1:13 p.m., 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. > > > > Why we want a PMU parent? Maybe I missed it. I don't see that the parent > > is used anywhere. > > This allows you to identify the association between PMU and the hardware related > device that is providing it by looking at the directory structure in sysfs rather > than putting them directly under /sys/devices. > > ls -l /sys/bus/event_sources/devices/ > > ... armv8_pmuv3_0 -> ../../../devices/arm8_pmuv3_0 > ... breakpoint -> ../../../devices/breakpoint > ... cpmu0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/cpmu0/cpmu0 > etc > > (the first cpmu0 is the parent registered as a child of the PCI EP and used for > driver binding). So it's of use to userspace rather than in the kernel driver > itself. > > Note that almost nothing is normally in the top level /sys/devices other than > event_sources - because nearly all other struct device instances created by > subsystems have parents assigned. > > On my system > > ls /sys/devices > > armv8_pmuv3_0 LNXSYSTEM:00 pci0000:0c pnp0 system uprobe > breakpoint pci0000:00 platform software tracepoint virtual > > +CC Greg KH for input on whether / why this make sense. That doesn't make sense, nothing should be in /sys/devices/ EXCEPT the root device of busses. Everything else is wrong and should have their code fixed up (i.e. "breakpoint", "software", etc.) thanks, greg k-h
On Tue, 28 Mar 2023 13:01:08 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Mar 28, 2023 at 11:54:44AM +0100, Jonathan Cameron wrote: > > On Mon, 27 Mar 2023 13:04:08 -0400 > > "Liang, Kan" <kan.liang@linux.intel.com> wrote: > > > > > On 2023-03-24 1:13 p.m., 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. > > > > > > Why we want a PMU parent? Maybe I missed it. I don't see that the parent > > > is used anywhere. > > > > This allows you to identify the association between PMU and the hardware related > > device that is providing it by looking at the directory structure in sysfs rather > > than putting them directly under /sys/devices. > > > > ls -l /sys/bus/event_sources/devices/ > > > > ... armv8_pmuv3_0 -> ../../../devices/arm8_pmuv3_0 > > ... breakpoint -> ../../../devices/breakpoint > > ... cpmu0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/cpmu0/cpmu0 > > etc > > > > (the first cpmu0 is the parent registered as a child of the PCI EP and used for > > driver binding). So it's of use to userspace rather than in the kernel driver > > itself. > > > > Note that almost nothing is normally in the top level /sys/devices other than > > event_sources - because nearly all other struct device instances created by > > subsystems have parents assigned. > > > > On my system > > > > ls /sys/devices > > > > armv8_pmuv3_0 LNXSYSTEM:00 pci0000:0c pnp0 system uprobe > > breakpoint pci0000:00 platform software tracepoint virtual > > > > +CC Greg KH for input on whether / why this make sense. > > That doesn't make sense, nothing should be in /sys/devices/ EXCEPT the > root device of busses. Everything else is wrong and should have their > code fixed up (i.e. "breakpoint", "software", etc.) Thanks Greg. I was thinking to cycle back round to that once I'd got agreement on 'some' devices, but great to have clarity from the start that these should all have parents. For a few cases the parent is not immediately obvious but we'll figure it out. Jonathan > > thanks, > > greg k-h
On Wed, Mar 29, 2023 at 09:55:04AM +0100, Jonathan Cameron wrote: > On Tue, 28 Mar 2023 13:01:08 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Tue, Mar 28, 2023 at 11:54:44AM +0100, Jonathan Cameron wrote: > > > On Mon, 27 Mar 2023 13:04:08 -0400 > > > "Liang, Kan" <kan.liang@linux.intel.com> wrote: > > > > > > > On 2023-03-24 1:13 p.m., 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. > > > > > > > > Why we want a PMU parent? Maybe I missed it. I don't see that the parent > > > > is used anywhere. > > > > > > This allows you to identify the association between PMU and the hardware related > > > device that is providing it by looking at the directory structure in sysfs rather > > > than putting them directly under /sys/devices. > > > > > > ls -l /sys/bus/event_sources/devices/ > > > > > > ... armv8_pmuv3_0 -> ../../../devices/arm8_pmuv3_0 > > > ... breakpoint -> ../../../devices/breakpoint > > > ... cpmu0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/cpmu0/cpmu0 > > > etc > > > > > > (the first cpmu0 is the parent registered as a child of the PCI EP and used for > > > driver binding). So it's of use to userspace rather than in the kernel driver > > > itself. > > > > > > Note that almost nothing is normally in the top level /sys/devices other than > > > event_sources - because nearly all other struct device instances created by > > > subsystems have parents assigned. > > > > > > On my system > > > > > > ls /sys/devices > > > > > > armv8_pmuv3_0 LNXSYSTEM:00 pci0000:0c pnp0 system uprobe > > > breakpoint pci0000:00 platform software tracepoint virtual > > > > > > +CC Greg KH for input on whether / why this make sense. > > > > That doesn't make sense, nothing should be in /sys/devices/ EXCEPT the > > root device of busses. Everything else is wrong and should have their > > code fixed up (i.e. "breakpoint", "software", etc.) > > Thanks Greg. > > I was thinking to cycle back round to that once I'd got agreement on 'some' devices, > but great to have clarity from the start that these should all have > parents. > > For a few cases the parent is not immediately obvious but we'll figure it out. If there is no "real" parent, then it is a virtual device and belongs under the "virtual" directory. Otherwise why are these "devices" at all with such generic names? thanks, greg k-h
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; pmu->dev->release = pmu_dev_release; ret = dev_set_name(pmu->dev, "%s", pmu->name);
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> --- include/linux/perf_event.h | 1 + kernel/events/core.c | 1 + 2 files changed, 2 insertions(+)