Re: [PATCH v2 2/5] perf: Allow a PMU to have a parent.

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: <linux-cxl@vger.kernel.org>, <peterz@infradead.org>,
	<mingo@redhat.com>, <acme@kernel.org>, <mark.rutland@arm.com>,
	<will@kernel.org>, <dan.j.williams@intel.com>,
	<linuxarm@huawei.com>, <linux-perf-users@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	"Davidlohr Bueso" <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>, <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2 2/5] perf: Allow a PMU to have a parent.
Date: Tue, 28 Mar 2023 11:54:44 +0100	[thread overview]
Message-ID: <20230328115444.000036ea@Huawei.com> (raw)
In-Reply-To: <f8123e7c-36a6-a302-1101-e778622dc997@linux.intel.com>

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);  
> 