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

[Mi, Dapeng]

On 5/13/2026 1:35 AM, Chen, Zide wrote:
>
> 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

I see. Thanks.


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