Re: [PATCH 2/3] PCI: dwc: Use cached PCIe capability offset in core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling `dw_pcie_get_pcie_cap()` before `ops->init()`/`ops->pre_init()` accesses unclocked hardware registers, causing a kernel panic or system hang.
- [High] Direct access to `pci->pcie_cap` bypasses lazy initialization, leading to invalid register reads/writes and silent PCI config space corruption.
--

commit 26cd77098a4771b4de6b7f3b4769d54179db8507
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Use cached PCIe capability offset in core

This commit modifies the DWC core functions to use a cached pcie_cap offset
rather than looking up the capability each time.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1390,6 +1390,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	ep->msi_msg_addr = 0;
>  	ep->msi_map_size = 0;
>  
> +	dw_pcie_get_pcie_cap(pci);
> +
>  	epc = devm_pci_epc_create(dev, &epc_ops);

Will this dw_pcie_get_pcie_cap() call cause a crash by accessing unclocked
hardware registers?

Looking at dw_pcie_ep_init(), clocks and regulators aren't enabled until
ep->ops->pre_init(ep) is executed further down in this function.

Calling dw_pcie_get_pcie_cap() here performs MMIO reads on the DBI
configuration space. On architectures like ARM/ARM64, reading from unclocked
MMIO space can trigger a synchronous external abort and kernel panic. If the
bus instead returns 0xFFFFFFFF, it could loop infinitely searching for the
next capability.

[ ... ]

> 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
> @@ -575,6 +575,8 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	raw_spin_lock_init(&pp->lock);
>  
> +	dw_pcie_get_pcie_cap(pci);
> +
>  	bridge = devm_pci_alloc_host_bridge(dev, 0);

Does this have the same early MMIO access issue as dw_pcie_ep_init()?

The platform-specific pp->ops->init(pp) callback isn't called until after
bridge allocation and resource fetching, so clocks might not be ready yet.

[ ... ]

> @@ -1218,7 +1220,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  
>  int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  {
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;

Is it safe to bypass the lazy initialization provided by
dw_pcie_get_pcie_cap(pci)?

If pci->pcie_cap is 0, this direct struct access means the fallback
capability search is completely skipped.

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -807,8 +807,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  	if (pci->max_link_speed > 2)
>  		msleep(PCIE_RESET_CONFIG_WAIT_MS);
>  
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> +	val = dw_pcie_readw_dbi(pci, pci->pcie_cap + PCI_EXP_LNKSTA);

Could bypassing dw_pcie_get_pcie_cap() here lead to silent PCI config
space corruption?

If pci->pcie_cap remains 0, reading or writing to pci->pcie_cap +
PCI_EXP_LNKSTA would operate on offset 0x12. This maps to the standard PCI
Header Type and BIST registers rather than the PCIe capability region,
potentially breaking link status checking and speed configuration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509135152.2241235-1-18255117159@163.com?part=2