Re: [PATCH v3 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset

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

New issues:
- [High] The DWC core code unconditionally uses `pci->pcie_cap` without checking if it is zero, leading to silent PCI configuration space corruption, particularly in endpoint (EP) mode where it remains uninitialized for many drivers.
- [Low] An unused variable and redundant DBI read were left behind in `dw_pcie_link_get_max_link_width()` during the `pci->pcie_cap` refactoring.

Pre-existing issues:
- [High] Missing cleanup of chained IRQ handler and IRQ domain on probe failure causes Use-After-Free.
--

--- Patch [6]: [PATCH v3 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset ---
commit 66fc930727ddec47d08c4dc09e796cb0d73aa5a9
Author: Hans Zhang <[email protected]>

    PCI: dwc: rockchip: Use cached PCIe capability offset
    
    dw_pcie_host_init() calls .init before caching the offset. So inside
    .init we call dw_pcie_get_pcie_cap() to trigger caching. The helper will
    perform the DBI read (hardware is already enabled) and cache the result.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 731d93663ccae..be8b6187913df 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -369,7 +369,7 @@ static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>  	u32 cap, lnkcap;
>  
>  	/* Enable L0S capability for all SoCs */
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	cap = dw_pcie_get_pcie_cap(pci);
>  	if (cap) {
>  		lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
>  		lnkcap |= PCI_EXP_LNKCAP_ASPM_L0S;

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer and
cause a use-after-free on probe failure?

drivers/pci/controller/dwc/pcie-dw-rockchip.c:
rockchip_pcie_host_init() {
    ...
	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
					 rockchip);
    ...
}

The chained handler is registered with the rockchip devres-managed structure.
If dw_pcie_host_init() fails later in the probe process (for example, while
waiting for the link to come up), the devres framework frees the rockchip
structure.

Since rockchip_pcie_host_ops lacks a .deinit callback to unregister the
chained handler and free the domain, will the handler dereference the freed
rockchip struct if the legacy IRQ fires afterward?

[Severity: High]
Can this corrupt the PCI configuration space if pci->pcie_cap is 0?

drivers/pci/controller/dwc/pcie-designware-ep.c:
dw_pcie_ep_init_non_sticky_registers() {
    ...
	dw_pcie_setup(pci);
    ...
}

drivers/pci/controller/dwc/pcie-designware.c:
dw_pcie_link_set_max_speed() {
    ...
	u8 offset = pci->pcie_cap;

	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
    ...
}

If an EP driver does not call dw_pcie_get_pcie_cap() during its .init
callback, pci->pcie_cap remains 0. The dw_pcie_ep_init_non_sticky_registers()
function unconditionally calls dw_pcie_setup() without verifying if
pci->pcie_cap was initialized.

Does this blindly use offset 0 instead of the PCIe capability structure,
permanently corrupting critical PCI config registers (like Cache Line Size,
Latency Timer, Header Type, or BIST) when offset 0 is accessed?

This also seems contrary to the commit message's claim that a warning would
be printed and the operation skipped, as the DWC core code uses the offset
unconditionally.

[Severity: Low]
Is an unused variable left behind, causing a redundant DBI read?

drivers/pci/controller/dwc/pcie-designware.c:
dw_pcie_link_get_max_link_width() {
	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
    ...
}

The cap variable is initialized via dw_pcie_find_capability() but is never
used, while the function proceeds to use pci->pcie_cap.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic