Re: [PATCH 2/2] PCI: ultrarisc: get and enable DP1000 PCIe clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] If dw_pcie_resume_noirq() fails, the driver disables the clocks, which will cause a bus fault and kernel crash on the next system suspend when the DWC core attempts to read the DBI registers.
--

commit 7bdde7bd97edc4ae25c337b51b52e18bb46e6424
Author: Jia Wang <wangjia@ultrarisc.com>

PCI: ultrarisc: get and enable DP1000 PCIe clocks

This commit adds the required core, dbi, and aux clocks for the DP1000 PCIe
controller and enables them before initializing the DesignWare host. It also
manages the clocks across system suspend and resume.

> diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> index 6ee661ceff67a..74010a86244e7 100644
> --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
[ ... ]
> @@ -135,16 +174,37 @@ static int ultrarisc_pcie_probe(struct platform_device *pdev)
[ ... ]
>  static int ultrarisc_pcie_suspend_noirq(struct device *dev)
>  {
> -	struct dw_pcie *pci = dev_get_drvdata(dev);
> +	struct ultrarisc_pcie *ultra = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = &ultra->pci;
> +	int ret;
> +
> +	ret = dw_pcie_suspend_noirq(pci);
> +	if (ret)
> +		return ret;
>  
> -	return dw_pcie_suspend_noirq(pci);
> +	if (pci->suspended)
> +		clk_bulk_disable_unprepare(ARRAY_SIZE(ultra->clks), ultra->clks);
> +
> +	return 0;
>  }
>  
>  static int ultrarisc_pcie_resume_noirq(struct device *dev)
>  {
> -	struct dw_pcie *pci = dev_get_drvdata(dev);
> +	struct ultrarisc_pcie *ultra = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = &ultra->pci;
> +	int ret;
> +
> +	if (pci->suspended) {
> +		ret = clk_bulk_prepare_enable(ARRAY_SIZE(ultra->clks), ultra->clks);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = dw_pcie_resume_noirq(pci);
> +	if (ret && pci->suspended)
> +		clk_bulk_disable_unprepare(ARRAY_SIZE(ultra->clks), ultra->clks);

[Severity: High]
Will this cause a kernel crash on the next system suspend if
dw_pcie_resume_noirq() fails?

If dw_pcie_resume_noirq() fails (for example, due to a link training
timeout), it returns an error but leaves pci->suspended as true. This error
path then disables the clocks.

However, the PM core continues and marks the device as resumed. On the next
system suspend, ultrarisc_pcie_suspend_noirq() will unconditionally call
dw_pcie_suspend_noirq().

Since dw_pcie_suspend_noirq() accesses DBI registers by calling
dw_pcie_link_up(), and the clocks were left disabled, does this read
trigger a synchronous external abort and crash the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-ultrarisc-pci-clk-v1-0-5ea3308fdab3@ultrarisc.com?part=2