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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f8a2e42-f6ed-4328-9457-5f986d761224@arm.com>
Date:   Thu, 26 Oct 2023 17:52:20 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Shuai Xue <xueshuai@...ux.alibaba.com>
Cc:     Will Deacon <will@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>,
        Yicong Yang <yangyicong@...wei.com>,
        chengyou@...ux.alibaba.com, kaishen@...ux.alibaba.com,
        baolin.wang@...ux.alibaba.com, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
        rdunlap@...radead.org, mark.rutland@....com,
        zhuo.song@...ux.alibaba.com, renyu.zj@...ux.alibaba.com
Subject: Re: [PATCH v9 3/4] drivers/perf: add DesignWare PCIe PMU driver

On 26/10/2023 2:44 pm, Jonathan Cameron wrote:
> On Tue, 24 Oct 2023 17:29:34 +0800
> Shuai Xue <xueshuai@...ux.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.