Re: [PATCH v9 3/3] PCI: Remove device links to PHY

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The Keystone PCIe driver completely skips deinitializing the DesignWare host or endpoint during module removal, leading to severe resource leaks and Use-After-Free conditions.
--

--- Patch [3]: [PATCH v9 3/3] PCI: Remove device links to PHY ---
commit dba6a718906e10ffe3960ba8d21e61e4e4e62e26
Author: Vladimir Oltean <[email protected]>
Subject: PCI: Remove device links to PHY

This commit removes explicit device links between the PCIe controllers and
PHY providers in Cadence, DRA7xx, and Keystone drivers. The PHY core has
automatically added consumer device link support since 2019, making the
manual link management redundant.

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index e9b012a5ea9a2..8085c4f614031 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
[ ... ]
> @@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)
>  	pm_runtime_disable(dev);
>  	ks_pcie_disable_phy(ks_pcie);
>  
> -err_link:
> -	while (--i >= 0 && link[i])
> -		device_link_del(link[i]);
> -
> +err:
>  	return ret;
>  }
>  
>  static void ks_pcie_remove(struct platform_device *pdev)
>  {
>  	struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
> -	struct device_link **link = ks_pcie->link;
> -	int num_lanes = ks_pcie->num_lanes;
>  	struct device *dev = &pdev->dev;
>  
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
>  	ks_pcie_disable_phy(ks_pcie);

[Severity: High]
This is a pre-existing issue, but is the driver missing a call to
deregister the dw_pcie core during module removal?

The ks_pcie_remove() function does not call dw_pcie_host_deinit() or
dw_pcie_ep_deinit(). This could leave the PCI controller registered in the
kernel's PCI subsystem while its backing memory (allocated via
devm_kzalloc) is freed by devres when the driver is unbound. 

If this happens, wouldn't subsequent access to the PCI bus (e.g. via sysfs
or background polling) dereference freed pointers and cause a
use-after-free panic?

> -	while (num_lanes--)
> -		device_link_del(link[num_lanes]);
>  }
>  
>  static struct platform_driver ks_pcie_driver = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3