Re: [PATCH v2 2/3] 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] Unconditional register access in suspend after a prior resume failure causes kernel panic.
--

commit 91b0a79d0e5b5b1e0d3c9731fd011273f668f0a9
Author: Jia Wang <wangjia@ultrarisc.com>

PCI: ultrarisc: get and enable DP1000 PCIe clocks

Add the required core, dbi, and aux clocks for the DP1000 PCIe
controller and enable them before initializing the DesignWare host.
Also manage 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..6ed244770d538 100644
> --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
[ ... ]
> @@ -135,14 +174,31 @@ 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);

[Severity: High]
What happens here if a previous resume attempt failed to enable the clocks?

Looking at ultrarisc_pcie_resume_noirq() below, if clk_bulk_prepare_enable()
fails, the function returns an error while the clocks remain disabled and
pci->suspended remains true.

If the system then undergoes another suspend cycle, this function will call
dw_pcie_suspend_noirq(), which accesses hardware registers via
dw_pcie_link_up().

Since the clocks are disabled, could this register access cause a bus error
and subsequent kernel panic?

Should we check if the device is already suspended (or if the clocks are
already off) before calling dw_pcie_suspend_noirq()?

> +	if (ret)
> +		return ret;
> +
> +	if (pci->suspended)
> +		clk_bulk_disable_unprepare(ARRAY_SIZE(ultra->clks), ultra->clks);
>  
> -	return dw_pcie_suspend_noirq(pci);
> +	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;
> +	}
>  
>  	return dw_pcie_resume_noirq(pci);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-ultrarisc-pci-clk-v2-0-2f3fb6fba66b@ultrarisc.com?part=2