Re: [PATCH v8 phy-next 09/31] PCI: Remove device links to PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Critical memory corruption in cdns_plat_pcie_shutdown() due to incorrect pointer cast.
- [High] Asynchronous pm_runtime_put() is called immediately before pm_runtime_disable(), cancelling suspend transitions.
- [High] Shared IRQ handlers access hardware registers without checking PM runtime state, causing spurious interrupts or bus errors.
--

commit abda94a24ef1bb0817706ae87aa20f04251c14ad
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

PCI: Remove device links to PHY

This commit removes device links between the PCIe controllers and PHY
providers in the Cadence, DRA7xx, and Keystone drivers. The device links
are now redundant since the PHY core automatically adds device links to
order suspend/resume operations.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> index 8b12a46b56019..6fd16e09e2444 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> @@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
>  	struct pci_host_bridge *bridge;
>  	struct cdns_pcie_ep *ep;
>  	struct cdns_pcie_rc *rc;
> -	int phy_count;
>  	bool is_rc;
>  	int ret;

In cdns_plat_pcie_probe(), the driver data is initialized to point to a
struct cdns_plat_pcie object:

	cdns_plat_pcie = devm_kzalloc(dev, sizeof(*cdns_plat_pcie), GFP_KERNEL);
	...
	platform_set_drvdata(pdev, cdns_plat_pcie);

However, in cdns_plat_pcie_shutdown(), the pointer is extracted and cast
directly to struct cdns_pcie *:

static void cdns_plat_pcie_shutdown(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct cdns_pcie *pcie = dev_get_drvdata(dev);
	...
	cdns_pcie_disable_phy(pcie);
}

Since struct cdns_plat_pcie is only 8 bytes large, but the phy_count field
is at an offset of roughly 36 bytes in struct cdns_pcie, does this lead to
an out-of-bounds read and memory corruption when cdns_pcie_disable_phy()
attempts to iterate over the PHYs?

[ ... ]
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index fe4836925c4e3..cab9fad212add 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);

Does calling pm_runtime_put() immediately before pm_runtime_disable() cancel
the suspend transition?

pm_runtime_put() queues an asynchronous idle or suspend work item, but
calling pm_runtime_disable() immediately afterward cancels this pending
transition via __pm_runtime_barrier().

Could this leave the hardware in an active or incorrect power state from
the controller's perspective? Would it be safer to use synchronous
pm_runtime_put_sync() instead?

This asynchronous put pattern also appears in the error paths of
dra7xx_pcie_probe() and ks_pcie_probe().

While reviewing the power management handling in these drivers, I also
noticed that the shared interrupt handlers read hardware registers directly
without verifying if the device is active:

drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_irq_handler() {
	...
	reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);

If another device sharing the same IRQ line triggers an interrupt while the
PCIe controller is runtime suspended, will these handlers attempt to read
powered-down hardware?

Should the handlers check the PM runtime state using
pm_runtime_get_if_active() to prevent spurious interrupts or bus errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505100523.1922388-1-vladimir.oltean@nxp.com?part=9