Re: [PATCH 1/2] PCI: qcom: Implement .reset_root_port() for firmware-managed controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] pci: qcom_pcie_fw_reset_root_port fails unconditionally if CONFIG_PM_SLEEP=n
- [High] pci: qcom: MMIO crash due to PM state corruption via pm_runtime_force_suspend()
- [High] pci/ecam: memory corruption due to cfg->win assignment on 32-bit
- [Medium] pm: async pm_runtime_put() races with pm_runtime_disable() on probe error path
- [Medium] pci: dwc: lockless dw_pcie_msi_init() races with IRQ mask updates
--

commit 758a2a2a75785b89801fabff2d094acd8d02d3de
Author: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>

PCI: qcom: Implement .reset_root_port() for firmware-managed controllers

During PCIe recovery events, such as a fatal AER event or other
platform-specific PCIe failures, the controller may enter a state that
requires a full reset before normal link operation can be restored.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d903f10211c23..744455ae90d46 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1855,6 +1855,35 @@ static int qcom_pcie_set_max_opp(struct device *dev)
>  	return ret;
>  }
>  
> +/*
> + * Reset for firmware-managed controllers: power-cycle via runtime PM and
> + * reprogram DWC MSI registers on resume.
> + */
> +static int qcom_pcie_fw_reset_root_port(struct pci_host_bridge *bridge,
> +					struct pci_dev *pdev)
> +{
> +	struct pci_config_window *cfg = bridge->sysdata;
> +	struct device *dev = bridge->dev.parent;
> +	struct dw_pcie_rp *pp = cfg->priv;
> +	int ret;
> +
> +	ret = pm_runtime_force_suspend(dev);

[Severity: High]
Is there a risk of an SError or kernel panic here due to unclocked or
unpowered MMIO access?

Calling pm_runtime_force_suspend() physically powers off the PM domain.
However, because the controller has active children (PCI devices),
pm_runtime_need_not_resume() evaluates to false, leaving the device's
runtime_status as RPM_ACTIVE while disable_depth is > 0.

If a concurrent thread (like unprivileged userspace reading sysfs) calls
pci_config_pm_runtime_get(), rpm_resume() will see disable_depth > 0
and runtime_status == RPM_ACTIVE. It will incorrectly return success
without powering on the device, and the subsequent MMIO access will hit
powered-off hardware.

> +	if (ret) {
> +		dev_err(dev, "Failed to force runtime suspend for reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pm_runtime_force_resume(dev);

[Severity: High]
Does this fail unconditionally on systems where CONFIG_PM_SLEEP is disabled?

In include/linux/pm_runtime.h, if CONFIG_PM_SLEEP is disabled,
pm_runtime_force_resume() is a static inline function that
unconditionally returns -ENXIO.

This guarantees that PCIe root port resets, such as during AER recovery,
will permanently fail and leave the hardware powered off.

> +	if (ret) {
> +		dev_err(dev, "Failed to force runtime resume after reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dw_pcie_msi_init(pp);

[Severity: High]
Could this lead to memory corruption on 32-bit architectures?

On 32-bit systems, pci_ecam.c sets per_bus_mapping to true and allocates
cfg->winp as a heap pointer array using kcalloc(). Because cfg->win
and cfg->winp are union members in struct pci_config_window, cfg->win
ends up holding the virtual address of the heap-allocated pointer array.

qcom_pcie_ecam_host_init() unconditionally assigns pci->dbi_base =
cfg->win. When this new reset path calls dw_pcie_msi_init(pp), it uses
pci->dbi_base to write MSI configuration values, which will overwrite
the heap array instead of MMIO registers.

[Severity: Medium]
Is it safe to call dw_pcie_msi_init() locklessly from a concurrent AER
recovery context?

dw_pcie_msi_init() reads pp->irq_mask[ctrl] locklessly and writes it
directly to the hardware register:

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_msi_init() {
    ...
    dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK + ..., pp->irq_mask[ctrl]);
    ...
}

During AER recovery, this executes from a normal workqueue. If an IRQ
affinity change or driver unmask operation concurrently invokes
dw_pci_bottom_mask() on another CPU, it updates the mask under
pp->lock. The lockless read here could overwrite the hardware register
with a stale mask, desynchronizing the hardware and software states.

> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -2292,6 +2323,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		bridge->sysdata = cfg;
>  		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
>  		bridge->msi_domain = true;
> +		bridge->reset_root_port = qcom_pcie_fw_reset_root_port;
>  
>  		ret = pci_host_probe(bridge);
>  		if (ret)

[Severity: Medium]
Does the error path for pci_host_probe() race with pm_runtime_disable()?

When pci_host_probe() fails, the code jumps to the err_pm_runtime_put
label:

err_pm_runtime_put:
    pm_runtime_put(dev);
    pm_runtime_disable(dev);

Since pm_runtime_put() is asynchronous, it queues idle/suspend work
that can be immediately canceled by the subsequent pm_runtime_disable()
before the device is powered down. This leaves the hardware active
and leaks power. Should this use pm_runtime_put_sync() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-pcie-controller-reset-v1-0-51779736cf12@oss.qualcomm.com?part=1