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

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

New issues:
- [Critical] In Endpoint (EP) mode, `pci->pcie_cap` remains uninitialized (0), causing `dw_pcie_setup()` to silently corrupt standard PCI header registers.

Pre-existing issues:
- [High] 32-bit read-modify-write (RMW) of 16-bit PCIe Control registers accidentally clears Write-1-to-Clear (W1C) bits in the adjacent Status registers.
--

--- Patch [16]: [PATCH v3 16/16] PCI: dwc: ultrarisc: Use cached PCIe capability offset ---
commit 4d9321bed75fe3c10535cf01ea0a69ce527cb616
Author: Hans Zhang <[email protected]>

PCI: dwc: ultrarisc: Use cached PCIe capability offset

This commit updates ultrarisc_pcie_host_init to use dw_pcie_get_pcie_cap
to obtain the PCIe capability offset, as the core has not cached it yet.
Hardware is already enabled before this point.

> diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> index 6ee661ceff67a..3f6e3ab0aa697 100644
> --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> @@ -49,7 +49,7 @@ static int ultrarisc_pcie_host_init(struct dw_pcie_rp *pp)
>  	FIELD_MODIFY(PORT_FLT_SF_MASK, &val, PORT_FLT_SF_VAL_64);
>  	dw_pcie_writel_dbi(pci, PCIE_TIMER_CTRL_MAX_FUNC_NUM, val);
>  
> -	cap_exp = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	cap_exp = dw_pcie_get_pcie_cap(pci);

[Severity: Critical]
Does caching the capability offset here expose an uninitialized value
in Endpoint mode?

If the broader patch series replaced dw_pcie_find_capability with direct
reads of pci->pcie_cap in the EP paths without initializing it,
pci->pcie_cap could remain 0.

For example, in drivers/pci/controller/dwc/pcie-designware-ep.c:

dw_pcie_ep_init_non_sticky_registers() {
    if (funcs > 1) {
        offset = pci->pcie_cap;
        func0_lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
        [ ... ]
}

If dw_pcie_get_pcie_cap() is not called before this in the EP
initialization sequence, offset evaluates to 0. This causes
dw_pcie_setup() to write into standard PCI header registers like Cache
Line Size or BIST instead of the PCIe capability space, silently
corrupting the header.

>  	val = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCTL2);
>  	FIELD_MODIFY(PCI_EXP_LNKCTL2_TLS, &val, PCI_EXP_LNKCTL2_TLS_16_0GT);
>  	dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCTL2, val);

[Severity: High]
This is a pre-existing issue, but does this 32-bit read-modify-write
sequence unintentionally clear Write-1-to-Clear bits in the adjacent
Status register?

When ultrarisc_pcie_host_init() reads and writes 32 bits starting at
PCI_EXP_LNKCTL2:

drivers/pci/controller/dwc/pcie-ultrarisc.c:ultrarisc_pcie_host_init()
    val = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCTL2);
    FIELD_MODIFY(PCI_EXP_LNKCTL2_TLS, &val, PCI_EXP_LNKCTL2_TLS_16_0GT);
    dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCTL2, val);

The read includes the 16-bit Link Control 2 register and the adjacent
16-bit Link Status 2 register. If any W1C bits are set in the status
register, writing the 32-bit value back will clear those events. This
can drop hardware events like Link Equalization Requests or Device Errors,
potentially breaking link training or Advanced Error Reporting.

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

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