[PATCH] perf/dwc_pcie: fix duplicate pci_dev devices

[PATCH] perf/dwc_pcie: fix duplicate pci_dev devices

[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 | 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;
+	}
+
 	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);
 	if (!name)
 		return -ENOMEM;
 
@@ -640,7 +651,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,
@@ -727,7 +738,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 int __init dwc_pcie_pmu_init(void)
-- 
2.39.2

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

[Shuai Xue]

在 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@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?

>   	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.

>   	if (!name)
>   		return -ENOMEM;
>   
> @@ -640,7 +651,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,
> @@ -727,7 +738,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 int __init dwc_pcie_pmu_init(void)

Thanks.

Best Regards,
Shuai

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

[yunhui cui]

Hi Shuai,

On Thu, Jan 23, 2025 at 5:50 PM Shuai Xue <xueshuai@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@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.

>
> >       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.

>
> >       if (!name)
> >               return -ENOMEM;
> >
> > @@ -640,7 +651,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,
> > @@ -727,7 +738,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 int __init dwc_pcie_pmu_init(void)
>
> Thanks.
>
> Best Regards,
> Shuai
>

Thanks,
Yunhui

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

[Shuai Xue]

在 2025/1/24 10:56, yunhui cui 写道:
> Hi Shuai,
> 
> On Thu, Jan 23, 2025 at 5:50 PM Shuai Xue <xueshuai@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@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.

Thanks.

Shuai

Re: [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@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@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@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

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

[Will Deacon]

On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > >>>        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".

As pointed out above, changing the name will break userspace. So it's
simply not an option, sorry.

Will

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

[yunhui cui]

Hi Will,

On Fri, Jan 24, 2025 at 8:30 PM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > > >>>        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".
>
> As pointed out above, changing the name will break userspace. So it's
> simply not an option, sorry.

Could you specify what in the userspace is broken and give an example?

>
> Will

Thanks,
Yunhui

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

[Will Deacon]

On Sun, Jan 26, 2025 at 09:54:35AM +0800, yunhui cui wrote:
> Hi Will,
> 
> On Fri, Jan 24, 2025 at 8:30 PM Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > > > >>>        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".
> >
> > As pointed out above, changing the name will break userspace. So it's
> > simply not an option, sorry.
> 
> Could you specify what in the userspace is broken and give an example?

Can't I just take the documented example from dwc_pcie_pmu.rst:

$# perf stat -a -e dwc_rootport_13018/Rx_PCIe_TLP_Data_Payload/

If I have a script that uses that command on a machine today, then
applying your kernel patch will break the script, no?

Will

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

[yunhui cui]

Hi Will,

On Tue, Jan 28, 2025 at 12:52 AM Will Deacon <will@kernel.org> wrote:
>
> On Sun, Jan 26, 2025 at 09:54:35AM +0800, yunhui cui wrote:
> > Hi Will,
> >
> > On Fri, Jan 24, 2025 at 8:30 PM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > > > > >>>        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".
> > >
> > > As pointed out above, changing the name will break userspace. So it's
> > > simply not an option, sorry.
> >
> > Could you specify what in the userspace is broken and give an example?
>
> Can't I just take the documented example from dwc_pcie_pmu.rst:
>
> $# perf stat -a -e dwc_rootport_13018/Rx_PCIe_TLP_Data_Payload/
>
> If I have a script that uses that command on a machine today, then
> applying your kernel patch will break the script, no?

Well, this is just a usage guide for the PCIe PMU and has no binding
with perf. In user space, the existing PCIe PMUs in the system should
be traversed first, and then perf stat/record ... can be executed.

It's okay not to change the name, but it's better to change
dwc_rootport_%x to dwc_rootport_%d to correspond with
platform_device_register_data(NULL, "dwc_pcie_pmu", ...). It'll be
more intuitive. What do you think?

>
> Will

Thanks,
Yunhui

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

[Will Deacon]

On Sat, Feb 01, 2025 at 05:51:21PM +0800, yunhui cui wrote:
> On Tue, Jan 28, 2025 at 12:52 AM Will Deacon <will@kernel.org> wrote:
> > On Sun, Jan 26, 2025 at 09:54:35AM +0800, yunhui cui wrote:
> > > On Fri, Jan 24, 2025 at 8:30 PM Will Deacon <will@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > > > > > >>>        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".
> > > >
> > > > As pointed out above, changing the name will break userspace. So it's
> > > > simply not an option, sorry.
> > >
> > > Could you specify what in the userspace is broken and give an example?
> >
> > Can't I just take the documented example from dwc_pcie_pmu.rst:
> >
> > $# perf stat -a -e dwc_rootport_13018/Rx_PCIe_TLP_Data_Payload/
> >
> > If I have a script that uses that command on a machine today, then
> > applying your kernel patch will break the script, no?
> 
> Well, this is just a usage guide for the PCIe PMU and has no binding
> with perf. In user space, the existing PCIe PMUs in the system should
> be traversed first, and then perf stat/record ... can be executed.

"should be". ha! You know we can't rely on userspace doing whatever we
think it "should" do.

> It's okay not to change the name, but it's better to change
> dwc_rootport_%x to dwc_rootport_%d to correspond with
> platform_device_register_data(NULL, "dwc_pcie_pmu", ...). It'll be
> more intuitive. What do you think?

No, as I said earlier, we cannot break userspace like this. Please stop
trying to rename things that cannot be changed.

Will

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

[yunhui cui]

Hi Will,

On Tue, Feb 4, 2025 at 8:31 PM Will Deacon <will@kernel.org> wrote:
>
> On Sat, Feb 01, 2025 at 05:51:21PM +0800, yunhui cui wrote:
> > On Tue, Jan 28, 2025 at 12:52 AM Will Deacon <will@kernel.org> wrote:
> > > On Sun, Jan 26, 2025 at 09:54:35AM +0800, yunhui cui wrote:
> > > > On Fri, Jan 24, 2025 at 8:30 PM Will Deacon <will@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 24, 2025 at 05:11:05PM +0800, yunhui cui wrote:
> > > > > > > >>>        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".
> > > > >
> > > > > As pointed out above, changing the name will break userspace. So it's
> > > > > simply not an option, sorry.
> > > >
> > > > Could you specify what in the userspace is broken and give an example?
> > >
> > > Can't I just take the documented example from dwc_pcie_pmu.rst:
> > >
> > > $# perf stat -a -e dwc_rootport_13018/Rx_PCIe_TLP_Data_Payload/
> > >
> > > If I have a script that uses that command on a machine today, then
> > > applying your kernel patch will break the script, no?
> >
> > Well, this is just a usage guide for the PCIe PMU and has no binding
> > with perf. In user space, the existing PCIe PMUs in the system should
> > be traversed first, and then perf stat/record ... can be executed.
>
> "should be". ha! You know we can't rely on userspace doing whatever we
> think it "should" do.
>
> > It's okay not to change the name, but it's better to change
> > dwc_rootport_%x to dwc_rootport_%d to correspond with
> > platform_device_register_data(NULL, "dwc_pcie_pmu", ...). It'll be
> > more intuitive. What do you think?
>
> No, as I said earlier, we cannot break userspace like this. Please stop
> trying to rename things that cannot be changed.
>

All right, I'll update it to v2.

> Will

Thanks,
Yunhui