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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] DWC endpoint core uses uninitialized `pci->pcie_cap` during setup, leading to severe PCI configuration space corruption.
- [Low] Unused variable `cap` in `dw_pcie_link_get_max_link_width()`, leading to redundant MMIO reads.
--

--- Patch [8]: [PATCH v3 08/16] PCI: dwc: fu740: Use cached PCIe capability offset ---
commit 184aa9285c97c29f412656a3e7f63a515288813c
Author: Hans Zhang <[email protected]>

PCI: dwc: fu740: Use cached PCIe capability offset

This patch series updates the DesignWare PCIe core and several controller
drivers to cache the PCIe capability offset. By storing the offset in
pci->pcie_cap, it eliminates the need to repeatedly search for the
capability during link setup and initialization.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7d2794945704e..14228be8bbce7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1246,7 +1246,7 @@ static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
>  	 * to all other functions as well.
>  	 */
>  	if (funcs > 1) {
> -		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		offset = pci->pcie_cap;

[Severity: Critical]
Since the generic endpoint initialization path never caches the capability
offset (unlike the Root Complex initialization path), doesn't this mean
pci->pcie_cap remains 0?

If it remains 0, this direct assignment will corrupt offset 0x0C when
reading and writing the capability.

Furthermore, earlier in this function:

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

Calling dw_pcie_setup() unconditionally invokes dw_pcie_link_set_max_speed()
and dw_pcie_link_set_max_link_width(). These functions now use pci->pcie_cap
which evaluates to 0, causing severe corruption of standard PCI header
registers (such as Header Type and Cache Line Size at offset 0x0C).

>  		func0_lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  		func0_lnkcap = FIELD_GET(PCI_EXP_LNKCAP_MLW |
>  					 PCI_EXP_LNKCAP_SLS, func0_lnkcap);

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index ec4722ed93032..810729b918928 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c

[ ... ]

> @@ -843,7 +842,7 @@ EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  {
>  	u32 cap, ctrl2, link_speed;
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;

[Severity: Critical]
When called from the endpoint initialization path via dw_pcie_setup(),
doesn't pci->pcie_cap evaluate to 0?

If so, this base offset resolves to standard PCI header bytes instead of
the PCIe capability registers, corrupting configuration space when
writing back the updated speed values.

>  
>  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  

[ ... ]

> @@ -890,7 +889,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
>  {
>  	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: Low]
Is the cap variable still needed here? 

It seems this performs a redundant MMIO search via dw_pcie_find_capability()
but the result is completely ignored since the next line directly accesses
pci->pcie_cap.

>  
>  	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
>  }

[ ... ]

> @@ -935,10 +933,9 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
>  
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: Critical]
Similar to dw_pcie_link_set_max_speed(), if this is called from the endpoint
setup path where pci->pcie_cap remains uninitialized, doesn't this result in
writes to standard PCI header registers instead of the PCIe capability?

>  	FIELD_MODIFY(PCI_EXP_LNKCAP_MLW, &lnkcap, num_lanes);
> -	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
> +	dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP, lnkcap);
>  }

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

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