Re: [PATCH v2 2/2] PCI: ultrarisc: Skip MEM outbound iATU setup on DP1000

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `.remove` callback in platform driver causes memory leaks, use-after-free, and unbalanced clocks on module unload.
- [High] Ignored return value of `dw_pcie_setup_rc()` during resume leads to unconditionally accessing uninitialized hardware.
- [High] Missing `dw_pcie_dbi_ro_wr_dis()` on the error path leaves DBI read-only registers writable.
--

commit c554fce09754058c518d7827f376e2e64ea19f5a
Author: Xincheng Zhang <zhangxincheng@ultrarisc.com>

PCI: ultrarisc: Skip MEM outbound iATU setup on DP1000

This patch sets the bypass_ob_mem_iatu flag for the DP1000 controller to
skip generic DWC MEM outbound iATU setup. This avoids the 16-window limit
and allows for a larger 64-bit MEM aperture.

> diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
[ ... ]
>  static struct platform_driver ultrarisc_pcie_driver = {
>  	.driver = {
>  		.name	= "ultrarisc-pcie",
>  		.of_match_table = ultrarisc_pcie_of_match,
>  		.suppress_bind_attrs = true,
>  		.pm = &ultrarisc_pcie_pm_ops,
>  	},
>  	.probe = ultrarisc_pcie_probe,
>  };
>  module_platform_driver(ultrarisc_pcie_driver);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to leave out
the .remove callback in the platform driver?

Since dw_pcie_host_init() is not devres-managed, unloading the module 
triggers devres release which frees driver memory and disables clocks, but
leaves the PCI host bridge and devices registered. Could this cause a
use-after-free and hardware hang if those devices are accessed later?

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
[ ... ]
>  	if (pp->bridge->child_ops == &dw_child_pcie_ops || pp->ecam_enabled) {
>  		ret = dw_pcie_iatu_setup(pp);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
[ ... ]
>  	dw_pcie_dbi_ro_wr_dis(pci);

[Severity: High]
This is a pre-existing issue, but does this error path bypass restoring the 
read-only write protection? 

The function dw_pcie_setup_rc() disables read-only protection at the 
beginning via dw_pcie_dbi_ro_wr_en(). If dw_pcie_iatu_setup() fails, it 
returns immediately. Does this permanently leave critical PCI hardware 
configuration registers writable?

[ ... ]
>  	if (pci->pp.ops->init) {
>  		ret = pci->pp.ops->init(&pci->pp);
>  		if (ret) {
>  			dev_err(pci->dev, "Host init failed: %d\n", ret);
>  			return ret;
>  		}
>  	}
>  
>  	dw_pcie_setup_rc(&pci->pp);
>  
>  	ret = dw_pcie_start_link(pci);
>  	if (ret)
>  		goto err_deinit;

[Severity: High]
This is a pre-existing issue, but what happens if dw_pcie_setup_rc() fails
during resume?

It appears the return value of dw_pcie_setup_rc() is completely ignored. 
If it encounters an error like an ATU enable timeout returning -EBUSY, the 
driver proceeds to call dw_pcie_start_link() and re-enable endpoints while 
the Root Complex hardware is incorrectly initialized. Could this lead to 
accessing uninitialized hardware and causing a synchronous external abort?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905025635.211390-1-zhangxincheng@ultrarisc.com?part=2