Re: [PATCH 1/4] perf/dwc_pcie: Fix registration issue in multi PCIe controller instances

[Krishna Chaitanya Chundru <quic_krichai@quicinc.com>]

On 8/15/2024 7:10 PM, Yicong Yang wrote:
> On 2024/7/31 12:23, Krishna chaitanya chundru wrote:
>> When there are multiple of instances of PCIe controllers, registration
>> to perf driver fails with this error.
>> sysfs: cannot create duplicate filename '/devices/platform/dwc_pcie_pmu.0'
>> CPU: 0 PID: 166 Comm: modprobe Not tainted 6.10.0-rc2-next-20240607-dirty
>> Hardware name: Qualcomm SA8775P Ride (DT)
>> Call trace:
>>   dump_backtrace.part.8+0x98/0xf0
>>   show_stack+0x14/0x1c
>>   dump_stack_lvl+0x74/0x88
>>   dump_stack+0x14/0x1c
>>   sysfs_warn_dup+0x60/0x78
>>   sysfs_create_dir_ns+0xe8/0x100
>>   kobject_add_internal+0x94/0x224
>>   kobject_add+0xa8/0x118
>>   device_add+0x298/0x7b4
>>   platform_device_add+0x1a0/0x228
>>   platform_device_register_full+0x11c/0x148
>>   dwc_pcie_register_dev+0x74/0xf0 [dwc_pcie_pmu]
>>   dwc_pcie_pmu_init+0x7c/0x1000 [dwc_pcie_pmu]
>>   do_one_initcall+0x58/0x1c0
>>   do_init_module+0x58/0x208
>>   load_module+0x1804/0x188c
>>   __do_sys_init_module+0x18c/0x1f0
>>   __arm64_sys_init_module+0x14/0x1c
>>   invoke_syscall+0x40/0xf8
>>   el0_svc_common.constprop.1+0x70/0xf4
>>   do_el0_svc+0x18/0x20
>>   el0_svc+0x28/0xb0
>>   el0t_64_sync_handler+0x9c/0xc0
>>   el0t_64_sync+0x160/0x164
>> kobject: kobject_add_internal failed for dwc_pcie_pmu.0 with -EEXIST,
>> don't try to register things with the same name in the same directory.
>>
>> This is because of having same bdf value for devices under two different
>> controllers.
>>
>> Update the logic to use sbdf which is a unique number in case of
>> multi instance also.
>>
>> Fixes: af9597adc2f1 ("drivers/perf: add DesignWare PCIe PMU driver")
> 
> Did you run into this on a QCOM platform with Patch 4/4 since there's
> multiple PCIe domains?
> 
Yes we ran this in QCOM platform where it has multiple PCIe instances.
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/perf/dwc_pcie_pmu.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
>> index c5e328f23841..c115348b8d53 100644
>> --- a/drivers/perf/dwc_pcie_pmu.c
>> +++ b/drivers/perf/dwc_pcie_pmu.c
>> @@ -556,10 +556,10 @@ static int dwc_pcie_register_dev(struct pci_dev *pdev)
>>   {
>>   	struct platform_device *plat_dev;
>>   	struct dwc_pcie_dev_info *dev_info;
>> -	u32 bdf;
>> +	u32 sbdf;
>>   
>> -	bdf = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> -	plat_dev = platform_device_register_data(NULL, "dwc_pcie_pmu", bdf,
>> +	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));
>>   
>>   	if (IS_ERR(plat_dev))
>> @@ -611,15 +611,15 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
>>   	struct pci_dev *pdev = plat_dev->dev.platform_data;
>>   	struct dwc_pcie_pmu *pcie_pmu;
>>   	char *name;
>> -	u32 bdf, val;
>> +	u32 sbdf, val;
>>   	u16 vsec;
>>   	int ret;
>>   
>>   	vsec = pci_find_vsec_capability(pdev, pdev->vendor,
>>   					DWC_PCIE_VSEC_RAS_DES_ID);
>>   	pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
>> -	bdf = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> -	name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", bdf);
>> +	sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
> 
> sbdf is also registerd as the id of the platform device in platform_device_register_data() above,
> can we use it directly here without encoding it again?
> 
> Thanks.
> 
ack.

- Krishna chaitanya.
>> +	name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf);
>>   	if (!name)
>>   		return -ENOMEM;
>>   
>> @@ -650,7 +650,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
>>   	ret = cpuhp_state_add_instance(dwc_pcie_pmu_hp_state,
>>   				       &pcie_pmu->cpuhp_node);
>>   	if (ret) {
>> -		pci_err(pdev, "Error %d registering hotplug @%x\n", ret, bdf);
>> +		pci_err(pdev, "Error %d registering hotplug @%x\n", ret, sbdf);
>>   		return ret;
>>   	}
>>   
>> @@ -663,7 +663,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
>>   
>>   	ret = perf_pmu_register(&pcie_pmu->pmu, name, -1);
>>   	if (ret) {
>> -		pci_err(pdev, "Error %d registering PMU @%x\n", ret, bdf);
>> +		pci_err(pdev, "Error %d registering PMU @%x\n", ret, sbdf);
>>   		return ret;
>>   	}
>>   	ret = devm_add_action_or_reset(&plat_dev->dev, dwc_pcie_unregister_pmu,
>>