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

[Robin Murphy]

On 26/10/2023 2:44 pm, Jonathan Cameron wrote:
> On Tue, 24 Oct 2023 17:29:34 +0800
> Shuai Xue <xueshuai at linux.alibaba.com> wrote:
> 
>> + Will, Jonathan, Bjorn and Yicong for probe and hotplug handing.
>>
>> On 2023/10/24 02:51, Robin Murphy wrote:
>>> On 2023-10-23 13:32, Will Deacon wrote:
>>> [...]
>>>>> +
>>>>> +    /*
>>>>> +     * The Group#1 event measures the amount of data processed in 16-byte
>>>>> +     * units. Simplify the end-user interface by multiplying the counter
>>>>> +     * at the point of read.
>>>>> +     */
>>>>> +    if (event_id >= 0x20 && event_id <= 0x23)
>>>>> +        return (((u64)hi << 32) | lo) << 4;
>>>>> +    else
>>>>> +        return (((u64)hi << 32) | lo);
>>>>
>>>> nit, but I think it would be clearer to do:
>>>>
>>>>      ret = ((u64)hi << 32) | lo;
>>>>
>>>>      /* ... */
>>>>      if (event_id >= 0x20 && event_id <= 0x23)
>>>>          ret <<= 4;
>>>
>>> Nit: "ret *= 16;" since the comment says it's multiplying a value, not moving a bitfield. The compiler already knows the most efficient way to implement constant multiplication.
>>
>> Cool, will use multiplication directly.
>>
>>>    
>>>>
>>>>      return ret;
>>>>   
>>> [...]
>>>>> +static int __init dwc_pcie_pmu_init(void)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
>>>>> +                      "perf/dwc_pcie_pmu:online",
>>>>> +                      dwc_pcie_pmu_online_cpu,
>>>>> +                      dwc_pcie_pmu_offline_cpu);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    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?

Thanks,
Robin.