[External] Re: [PATCH] perf/dwc_pcie: fix duplicate pci_dev devices

[yunhui cui]

Hi Shuai,

On Fri, Jan 24, 2025 at 2:48 PM Shuai Xue <xueshuai at linux.alibaba.com> wrote:
>
>
>
> 在 2025/1/24 10:56, yunhui cui 写道:
> > Hi Shuai,
> >
> > On Thu, Jan 23, 2025 at 5:50 PM Shuai Xue <xueshuai at linux.alibaba.com> wrote:
> >>
> >>
> >>
> >> 在 2025/1/23 15:48, Yunhui Cui 写道:
> >>> During platform_device_register, wrongly using struct device
> >>> pci_dev as platform_data caused a kmemdup copy of pci_dev. Worse
> >>> still, accessing the duplicated device leads to list corruption as its
> >>> mutex content (e.g., list, magic) remains the same as the original.
> >>>
> >>> Signed-off-by: Yunhui Cui <cuiyunhui at bytedance.com>
> >>> ---
> >>>    drivers/perf/dwc_pcie_pmu.c | 25 ++++++++++++++++++-------
> >>>    1 file changed, 18 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
> >>> index cccecae9823f..8b208f287a1f 100644
> >>> --- a/drivers/perf/dwc_pcie_pmu.c
> >>> +++ b/drivers/perf/dwc_pcie_pmu.c
> >>> @@ -565,9 +565,7 @@ static int dwc_pcie_register_dev(struct pci_dev *pdev)
> >>>        u32 sbdf;
> >>>
> >>>        sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
> >>> -     plat_dev = platform_device_register_data(NULL, "dwc_pcie_pmu", sbdf,
> >>> -                                              pdev, sizeof(*pdev));
> >>> -
> >>> +     plat_dev = platform_device_register_simple("platform_dwc_pcie", sbdf, NULL, 0);
> >>>        if (IS_ERR(plat_dev))
> >>>                return PTR_ERR(plat_dev);
> >>>
> >>> @@ -614,19 +612,32 @@ static struct notifier_block dwc_pcie_pmu_nb = {
> >>>
> >>>    static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
> >>>    {
> >>> -     struct pci_dev *pdev = plat_dev->dev.platform_data;
> >>> +     struct pci_dev *pdev = NULL;
> >>>        struct dwc_pcie_pmu *pcie_pmu;
> >>> +     bool found = false;
> >>>        char *name;
> >>>        u32 sbdf;
> >>>        u16 vsec;
> >>>        int ret;
> >>>
> >>> +     for_each_pci_dev(pdev) {
> >>> +             sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
> >>> +             if (sbdf == plat_dev->id) {
> >>> +                     found = true;
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +     if (!found) {
> >>> +             pr_err("The plat_dev->id does not match the sbdf");
> >>> +             return -ENODEV;
> >>> +     }
> >>> +
> >>
> >> How about using pci_get_domain_bus_and_slot() to find pci_dev?
> > It's not necessary. pci_get_domain_bus_and_slot also invokes
> > for_each_pci_dev, and it further requires splitting the sbdf.
>
> Compose sbdf from domain, bus number, and devfn is almost essentially
> corresponding opposite operation. And I think we should grab a reference
> count of pdev and handle it properly.
>
> Personally speaking, I prefer pci_get_domain_bus_and_slot() because it is
> simple and robust.
>
> >
> >>
> >>>        vsec = dwc_pcie_des_cap(pdev);
> >>>        if (!vsec)
> >>>                return -ENODEV;
> >>>
> >>>        sbdf = plat_dev->id;
> >>> -     name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf);
> >>> +     name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%d_pmu", sbdf);
> >>
> >> A new name will break previous user tools.
> > This name isn't suitable. It can't clearly show which is the PMU
> > device. Userspace tools don't have binding relationships, like perf.
> > Tools must traverse PMU devices before use.
>
> The device is under /sys/bus/event_source/ which indates it is PMU device.
> As far as I know, most of PMU devices do not endup with a '_pmu' prefix.

The point is the name "dwc_rootport_" is misleading, suggesting an
extra PCIE RP in the system. Best to change the name to intuitive
"xx_pmu".

>
> Thanks.
>
> Shuai
>

Thanks,
Yunhui