Re: [PATCH v9 3/4] drivers/perf: add DesignWare PCIe PMU driver

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 27 Oct 2023 20:25:16 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> On 2023/10/27 00:52, Robin Murphy wrote:
> > On 26/10/2023 2:44 pm, Jonathan Cameron wrote:  
> >> On Tue, 24 Oct 2023 17:29:34 +0800
> >> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> >>  
> >>> + Will, Jonathan, Bjorn and Yicong for probe and hotplug handing.
> >>>  
> ...
> >>>>>> +
> >>>>>> +    dwc_pcie_pmu_hp_state = ret;
> >>>>>> +
> >>>>>> +    ret = platform_driver_register(&dwc_pcie_pmu_driver);
> >>>>>> +    if (ret)
> >>>>>> +        goto platform_driver_register_err;
> >>>>>> +
> >>>>>> +    dwc_pcie_pmu_dev = platform_device_register_simple(
> >>>>>> +                "dwc_pcie_pmu", PLATFORM_DEVID_NONE, NULL, 0);
> >>>>>> +    if (IS_ERR(dwc_pcie_pmu_dev)) {
> >>>>>> +        ret = PTR_ERR(dwc_pcie_pmu_dev);
> >>>>>> +        goto platform_device_register_error;
> >>>>>> +    }  
> >>>>>
> >>>>> I'm a bit confused as to why you're having to create a platform device
> >>>>> for a PCI device -- is this because the main designware driver has already
> >>>>> bound to it? A comment here explaining why you need to do this would be
> >>>>> very helpful. In particular, is there any dependency on another driver
> >>>>> to make sure that e.g. config space accesses work properly? If so, we
> >>>>> probably need to enforce module load ordering or something like that.  
> >>>>
> >>>> AFAICS the platform device/driver serve no purpose other than being a hilariously roundabout way to run the for_each_pci_dev() loop in dwc_pcie_pmu_probe() upon module init, and to save explicitly freeing the PMU name/data. Furthermore the devres action for dwc_pcie_pmu_remove_cpuhp_instance() is apparently going for even more style points at module exit by not even relying on the corresponding .remove callback of the tenuous platform driver to undo what its .probe did, but (ab)using the device's devres list to avoid having to keep track of an explicit list of PMU instances at all.  
> >>>
> >>> You are right.  
> >>
> >> Also provides a (potential) parent for the PMU devices which is something
> >> we were trying to clean up for existing PMUs (which end up in the
> >> wrong directly in sysfs because they typically don't have parents).  
> > 
> > Surely the relevant PCI device would be an even more appropriate parent, though, since that's the true topology?
> >   
> 
> I see, I will add its parent.

Agreed - I hadn't it in my head that we didn't have a good mapping to a particular
PCIe device (based on some similarish hardware where the counters are shared across
multiple RPs) Here I guess it's fine to use the PCI device.

Jonathan

> 
> Thank you.
> Best Regards,
> Shuai
>