Re: [PATCH] PCI: keystone: Fix host bridge device leak in ks_pcie_quirk()

[Jan Kiszka <jan.kiszka@siemens.com>]

On 17.09.26 15:20, Wentao Liang wrote:
> ks_pcie_quirk() obtains a reference to the host bridge device with
> pci_get_host_bridge_device() but never releases it.  Every early exit
> from the AM654 PG1.0 workaround leaks the reference: when the bridge
> device or its parent is missing, when the parent has no driver data,
> and when the RTL revision is not AM6_PCI_PG1_RTL_VER.  The quirk runs
> for every downstream PCI device, so the reference count grows without
> bound.
> 
> Add a common out label that drops the reference with
> pci_put_host_bridge_device() before returning.
> 
> Fixes: 86f271f22bbb ("PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 278d2dba1db0..cc6739edd434 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -528,7 +528,7 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>  {
>  	struct pci_bus *bus = dev->bus;
>  	struct keystone_pcie *ks_pcie;
> -	struct device *bridge_dev;
> +	struct device *bridge_dev = NULL;
>  	struct pci_dev *bridge;
>  	u32 val;
>  
> @@ -582,23 +582,27 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>  	if (pci_match_id(am6_pci_devids, bridge)) {
>  		bridge_dev = pci_get_host_bridge_device(dev);
>  		if (!bridge_dev || !bridge_dev->parent)
> -			return;
> +			goto out;
>  
>  		ks_pcie = dev_get_drvdata(bridge_dev->parent);
>  		if (!ks_pcie)
> -			return;
> +			goto out;
>  
>  		val = ks_pcie_app_readl(ks_pcie, PID);
>  		val &= RTL;
>  		val >>= RTL_SHIFT;
>  		if (val != AM6_PCI_PG1_RTL_VER)
> -			return;
> +			goto out;
>  
>  		if (pcie_get_readrq(dev) > 128) {
>  			dev_info(&dev->dev, "limiting MRRS to 128 bytes\n");
>  			pcie_set_readrq(dev, 128);
>  		}
>  	}
> +
> +out:
> +	if (bridge_dev)
> +		pci_put_host_bridge_device(bridge_dev);

I would pull this under the scope of pci_match_id.

>  }
>  DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, ks_pcie_quirk);
>  

Good catch, even more as it does not only affect the legacy silicon. But
I guess the leaking normally ends when all devices were probed and
enabled, and no one has a desire to unbind/rebind them.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center