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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Asynchronous `pm_runtime_put()` immediately followed by `pm_runtime_disable()` risks canceling pending suspend work, leaving hardware in an incorrect power state.
- [High] Missing cleanup of DWC host and endpoint structures in `ks_pcie_remove()` leads to a Use-After-Free vulnerability.
- [High] Hardware registers are unconditionally accessed in a shared IRQ handler without checking runtime PM state, risking a kernel panic.
- [High] Early registration of a shared IRQ before state structures are fully initialized introduces a race condition leading to a NULL pointer dereference.
--

commit cee8876507fd3272f25089c961cbca85b16e29eb
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

PCI: keystone: Remove device links to PHY

This commit partially reverts earlier device link handling to clean up
PHY provider and consumer suspend/resume operations. The PHY core now
handles the device link addition natively, making the local implementation
redundant.

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c

[ ... ]

> @@ -1338,25 +1322,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);

[Severity: High]
This is a pre-existing issue, but does using the asynchronous
pm_runtime_put() right before pm_runtime_disable() risk leaving the
hardware active?

According to PM subsystem guidelines, pm_runtime_put() queues asynchronous
suspend work which is immediately flushed and canceled by
pm_runtime_disable(). Should this use the synchronous pm_runtime_put_sync()
instead to ensure the device is properly suspended in both this remove path
and the probe error path?

[Severity: High]
This is also a pre-existing issue, but does ks_pcie_remove() miss the
required DWC core cleanup routines?

It looks like dw_pcie_host_init() or dw_pcie_ep_init() are called during
probe, but there are no symmetrical calls to dw_pcie_host_deinit() or
dw_pcie_ep_deinit() here. Since the struct dw_pcie is allocated with
devm_kzalloc(), wouldn't the active PCI core retain dangling references
after unbinding, leading to a Use-After-Free?

>  	ks_pcie_disable_phy(ks_pcie);
> -	while (num_lanes--)
> -		device_link_del(link[num_lanes]);
>  }

While reviewing the PM handling in this driver, I noticed a couple of
other pre-existing issues regarding the error IRQ handler.

[Severity: High]
This is a pre-existing issue, but could this early IRQ registration lead to
a NULL pointer dereference?

In ks_pcie_probe(), the shared IRQ is requested before ks_pcie->pci is
assigned:

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_probe() {
	...
	ret = devm_request_irq(dev, irq, ks_pcie_err_irq_handler, IRQF_SHARED,
			       "ks-pcie-error-irq", ks_pcie);
	...
	ks_pcie->pci = pci;
	...
}

If an interrupt from another device on the same shared line fires
immediately, the handler will run and dereference the uninitialized pointer:

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_handle_error_irq() {
	struct device *dev = ks_pcie->pci->dev;
	...
}

[Severity: High]
This is another pre-existing issue, but can the shared error IRQ handler
safely read hardware registers without checking the runtime PM state?

If another device sharing the IRQ line fires an interrupt while the
keystone controller is suspended and its clocks are gated, will reading
ERR_IRQ_STATUS cause a kernel panic (synchronous external abort) or return
invalid states?

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_handle_error_irq() {
	...
	reg = ks_pcie_app_readl(ks_pcie, ERR_IRQ_STATUS);
	...
}

Should this use pm_runtime_get_if_active() to ensure the device is clocked
before accessing hardware registers?

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