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

[Bjorn Helgaas <helgaas@kernel.org>]

[+to Manikandan, Siddharth, Vignesh, Hans]

On Thu, May 07, 2026 at 07:59:58AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Please take a look at these potential issues and see whether changes
are needed.

pcie-cadence-plat.c and pci-keystone.c are orphaned since

  0ee2030af4e3 ("MAINTAINERS: Orphan Cadence PCIe IP")
  57e1d8206e48 ("MAINTAINERS: move Murali Karicheri to credits")

Would anybody like to step up and take care of them?

> 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