| Message ID | 20250208104002.60332-3-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix dwc_pcie pmu driver issues | expand |
在 2025/2/8 18:40, 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@bytedance.com> > --- > drivers/perf/dwc_pcie_pmu.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c > index 19fa2ba8dd67..4f6599e32bba 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); > > @@ -616,18 +614,26 @@ 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; > struct dwc_pcie_pmu *pcie_pmu; > char *name; > u32 sbdf; > u16 vsec; > int ret; > > + sbdf = plat_dev->id; > + pdev = pci_get_domain_bus_and_slot(sbdf >> 16, PCI_BUS_NUM(sbdf & 0xffff), > + sbdf & 0xff); > + if (!pdev) { > + pr_err("No pdev found for the sbdf 0x%x\n", sbdf); > + return -ENODEV; > + } > + > vsec = dwc_pcie_des_cap(pdev); > if (!vsec) > return -ENODEV; pci_dev_put(pdev) should move ahead to aovid return here. > > - sbdf = plat_dev->id; > + pci_dev_put(pdev); > name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf); > if (!name) > return -ENOMEM; > @@ -642,7 +648,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev) > pcie_pmu->on_cpu = -1; > pcie_pmu->pmu = (struct pmu){ > .name = name, > - .parent = &pdev->dev, > + .parent = &plat_dev->dev, > .module = THIS_MODULE, > .attr_groups = dwc_pcie_attr_groups, > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > @@ -729,7 +735,7 @@ static int dwc_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_n > > static struct platform_driver dwc_pcie_pmu_driver = { > .probe = dwc_pcie_pmu_probe, > - .driver = {.name = "dwc_pcie_pmu",}, > + .driver = {.name = "platform_dwc_pcie",}, Aha, it is very difficult to come up with a name that satisfies everyone. The original name uses the '_pmu' suffix to follow the unwritten convention of other PMU drivers. Personally, I think the original name is more appropriate, but I'll leave the decision to @Will. Thanks. Best Regards. Shuai
Hi Shuai, On Tue, Feb 11, 2025 at 4:02 PM Shuai Xue <xueshuai@linux.alibaba.com> wrote: > > > > 在 2025/2/8 18:40, 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@bytedance.com> > > --- > > drivers/perf/dwc_pcie_pmu.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c > > index 19fa2ba8dd67..4f6599e32bba 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); > > > > @@ -616,18 +614,26 @@ 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; > > struct dwc_pcie_pmu *pcie_pmu; > > char *name; > > u32 sbdf; > > u16 vsec; > > int ret; > > > > + sbdf = plat_dev->id; > > + pdev = pci_get_domain_bus_and_slot(sbdf >> 16, PCI_BUS_NUM(sbdf & 0xffff), > > + sbdf & 0xff); > > + if (!pdev) { > > + pr_err("No pdev found for the sbdf 0x%x\n", sbdf); > > + return -ENODEV; > > + } > > + > > vsec = dwc_pcie_des_cap(pdev); > > if (!vsec) > > return -ENODEV; > > pci_dev_put(pdev) should move ahead to aovid return here. > > > > > - sbdf = plat_dev->id; > > + pci_dev_put(pdev); > > name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf); > > if (!name) > > return -ENOMEM; > > @@ -642,7 +648,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev) > > pcie_pmu->on_cpu = -1; > > pcie_pmu->pmu = (struct pmu){ > > .name = name, > > - .parent = &pdev->dev, > > + .parent = &plat_dev->dev, > > .module = THIS_MODULE, > > .attr_groups = dwc_pcie_attr_groups, > > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > > @@ -729,7 +735,7 @@ static int dwc_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_n > > > > static struct platform_driver dwc_pcie_pmu_driver = { > > .probe = dwc_pcie_pmu_probe, > > - .driver = {.name = "dwc_pcie_pmu",}, > > + .driver = {.name = "platform_dwc_pcie",}, > > Aha, it is very difficult to come up with a name that satisfies everyone. The > original name uses the '_pmu' suffix to follow the unwritten convention of > other PMU drivers. > > Personally, I think the original name is more appropriate, but I'll leave the > decision to @Will. Since Will hasn't replied, I'll update to the next version to keep the original name. > > Thanks. > Best Regards. > Shuai Thanks, Yunhui
diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c index 19fa2ba8dd67..4f6599e32bba 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); @@ -616,18 +614,26 @@ 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; struct dwc_pcie_pmu *pcie_pmu; char *name; u32 sbdf; u16 vsec; int ret; + sbdf = plat_dev->id; + pdev = pci_get_domain_bus_and_slot(sbdf >> 16, PCI_BUS_NUM(sbdf & 0xffff), + sbdf & 0xff); + if (!pdev) { + pr_err("No pdev found for the sbdf 0x%x\n", sbdf); + return -ENODEV; + } + vsec = dwc_pcie_des_cap(pdev); if (!vsec) return -ENODEV; - sbdf = plat_dev->id; + pci_dev_put(pdev); name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf); if (!name) return -ENOMEM; @@ -642,7 +648,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev) pcie_pmu->on_cpu = -1; pcie_pmu->pmu = (struct pmu){ .name = name, - .parent = &pdev->dev, + .parent = &plat_dev->dev, .module = THIS_MODULE, .attr_groups = dwc_pcie_attr_groups, .capabilities = PERF_PMU_CAP_NO_EXCLUDE, @@ -729,7 +735,7 @@ static int dwc_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_n static struct platform_driver dwc_pcie_pmu_driver = { .probe = dwc_pcie_pmu_probe, - .driver = {.name = "dwc_pcie_pmu",}, + .driver = {.name = "platform_dwc_pcie",}, }; static void dwc_pcie_cleanup_devices(void)
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@bytedance.com> --- drivers/perf/dwc_pcie_pmu.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)