Re: [PATCH] PCI: keystone: drop host bridge reference in ks_pcie_quirk()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI: keystone: unexported pci_put_host_bridge_device() breaks module build
--

commit 7a2a6fc28bf725e307c0f83095897f6ac6b29cc8
Author: Wentao Liang <vulab@iscas.ac.cn>

PCI: keystone: drop host bridge reference in ks_pcie_quirk()

This commit fixes a host bridge device reference leak in ks_pcie_quirk() by
calling pci_put_host_bridge_device() via a cleanup label on all exit paths
after pci_get_host_bridge_device().

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index edfbc32904cda..0c46887de0d5d 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -579,26 +579,33 @@ static void ks_pcie_quirk(struct pci_dev *dev)
>  	 * when MRRS is set to more than 128 bytes. Force the MRRS to
>  	 * 128 bytes in all downstream devices.
>  	 */
> -	if (pci_match_id(am6_pci_devids, bridge)) {
> -		bridge_dev = pci_get_host_bridge_device(dev);
> -		if (!bridge_dev || !bridge_dev->parent)
> -			return;
> -
> -		ks_pcie = dev_get_drvdata(bridge_dev->parent);
> -		if (!ks_pcie)
> -			return;
> -
> -		val = ks_pcie_app_readl(ks_pcie, PID);
> -		val &= RTL;
> -		val >>= RTL_SHIFT;
> -		if (val != AM6_PCI_PG1_RTL_VER)
> -			return;
> -
> -		if (pcie_get_readrq(dev) > 128) {
> -			dev_info(&dev->dev, "limiting MRRS to 128 bytes\n");
> -			pcie_set_readrq(dev, 128);
> -		}
> +	if (!pci_match_id(am6_pci_devids, bridge))
> +		return;
> +
> +	bridge_dev = pci_get_host_bridge_device(dev);
> +	if (!bridge_dev)
> +		return;
> +
> +	if (!bridge_dev->parent)
> +		goto put_bridge;
> +
> +	ks_pcie = dev_get_drvdata(bridge_dev->parent);
> +	if (!ks_pcie)
> +		goto put_bridge;
> +
> +	val = ks_pcie_app_readl(ks_pcie, PID);
> +	val &= RTL;
> +	val >>= RTL_SHIFT;
> +	if (val != AM6_PCI_PG1_RTL_VER)
> +		goto put_bridge;
> +
> +	if (pcie_get_readrq(dev) > 128) {
> +		dev_info(&dev->dev, "limiting MRRS to 128 bytes\n");
> +		pcie_set_readrq(dev, 128);
>  	}
> +
> +put_bridge:
> +	pci_put_host_bridge_device(bridge_dev);
>  }

[Severity: Low]
Will calling pci_put_host_bridge_device() here break the module build when
CONFIG_PCI_KEYSTONE_HOST=m or CONFIG_PCI_KEYSTONE_EP=m on non-ARM32 targets
(which selects CONFIG_PCI_KEYSTONE_TRISTATE=m)?

In drivers/pci/host-bridge.c, pci_get_host_bridge_device() is exported for
modules, but pci_put_host_bridge_device() is not:

drivers/pci/host-bridge.c:
    struct device *pci_get_host_bridge_device(struct pci_dev *dev)
    {
        ...
    }
    EXPORT_SYMBOL_GPL(pci_get_host_bridge_device);

    void  pci_put_host_bridge_device(struct device *dev)
    {
        kobject_put(&dev->kobj);
    }

Additionally, pci_put_host_bridge_device() is currently only declared in the
internal header drivers/pci/pci.h rather than include/linux/pci.h alongside
pci_get_host_bridge_device().

Should pci_put_host_bridge_device() be exported with EXPORT_SYMBOL_GPL() in
drivers/pci/host-bridge.c (and moved to include/linux/pci.h) so modular builds
of pci-keystone.ko do not fail at modpost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103104239.1373754-1-vulab@iscas.ac.cn?part=1