Re: [PATCH 2/6] perf/x86/intel/uncore: Fix PCI device refcount leak in UPI discovery

[Chen, Zide]

On 5/12/2026 2:27 AM, Mi, Dapeng wrote:
> 
> On 5/12/2026 7:05 AM, Zide Chen wrote:
>> pci_get_domain_bus_and_slot() increments the reference count of the
>> returned PCI device and therefore requires a matching pci_dev_put().
>>
>> In skx_upi_topology_cb() and discover_upi_topology(), the lookup is
>> performed inside a loop, but pci_dev_put() is only called once after
>> the loop. As a result, references from all previous iterations are
>> leaked.
>>
>> Move pci_dev_put(dev) into the if (dev) block immediately after
>> upi_fill_topology() returns.
>>
>> Opportunistically, fix uninitialized variable in skx_upi_topology_cb().
>>
>> Fixes: 4cfce57fa42d ("perf/x86/intel/uncore: Enable UPI topology discovery for Skylake Server")
>> Fixes: f680b6e6062e ("perf/x86/intel/uncore: Enable UPI topology discovery for Icelake Server")
>> Signed-off-by: Zide Chen <zide.chen@xxxxxxxxx>
>> ---
>>  arch/x86/events/intel/uncore_snbep.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
>> index 215d33e260ed..c9ce206fcbb6 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -4261,7 +4261,7 @@ static int upi_fill_topology(struct pci_dev *dev, struct intel_uncore_topology *
>>  static int skx_upi_topology_cb(struct intel_uncore_type *type, int segment,
>>  				int die, u64 cpu_bus_msr)
>>  {
>> -	int idx, ret;
>> +	int idx, ret = 0;
>>  	struct intel_uncore_topology *upi;
>>  	unsigned int devfn;
>>  	struct pci_dev *dev = NULL;
>> @@ -4274,12 +4274,12 @@ static int skx_upi_topology_cb(struct intel_uncore_type *type, int segment,
>>  		dev = pci_get_domain_bus_and_slot(segment, bus, devfn);
>>  		if (dev) {
>>  			ret = upi_fill_topology(dev, upi, idx);
>> +			pci_dev_put(dev);
>>  			if (ret)
>>  				break;
>>  		}
>>  	}
>>  
>> -	pci_dev_put(dev);
>>  	return ret;
>>  }
>>  
>> @@ -5499,6 +5499,7 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i
>>  							  devfn);
>>  			if (dev) {
>>  				ret = upi_fill_topology(dev, upi, idx);
>> +				pci_dev_put(dev);
>>  				if (ret)
>>  					goto err;
>>  			}
>> @@ -5506,7 +5507,6 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i
>>  	}
>>  err:
>>  	pci_dev_put(ubox);
>> -	pci_dev_put(dev);
> 
> Should we move the "pci_dev_put(ubox)" into the while loop as well? In
> theory, the ubox device could be found multiple times.

As mentioned below, pci_dev_put(ubox) is needed for the two 'goto err"
breaks. Moving it into the main loop would require two pci_dev_put()
calls, which adds no benefit.
 >
> Besides, could you please search "pci_get_device()" in uncore code, it
> seems some functions don't call pci_dev_put() or only calls it once, like
> the funciton spr_update_device_location() ...

pci_get_device() calls pci_dev_put() internally on the previous device
before returning the next one, so if the "while (pci_get_device())" loop
runs to completion without a break, no extra pci_dev_put() is needed:

https://elixir.bootlin.com/linux/v7.1-rc3/source/drivers/pci/search.c#L283


> Thanks.
> 
> 
> 
>>  	return ret;
>>  }
>>