Re: [PATCH v3 02/16] PCI: dwc: Use cached PCIe capability offset in core

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized `pci->pcie_cap` used in Endpoint mode causes severe PCIe configuration space corruption.
- [Low] Unnecessary PCIe capability search and unused variable left intact.
--

--- Patch [2]: [PATCH v3 02/16] PCI: dwc: Use cached PCIe capability offset in core ---
commit 1e2ce6ece477d5f9c179f2ac4c13cd06cf764f59
Author: Hans Zhang <[email protected]>

PCI: dwc: Use cached PCIe capability offset in core
    
Modify the DWC core functions to use the cached pcie_cap offset instead
of calling dw_pcie_find_capability() each time.

[ ... ]

> dw_pcie_ep_init_non_sticky_registers() now assumes pci->pcie_cap is
> valid. if not, it prints a warning and skips the operation. The other
> functions run after probe, so pci->pcie_cap is already valid and can
> be used directly.

[Severity: High]
The commit message mentions adding a warning and skipping the operation if
pci->pcie_cap is not valid, but this check appears to be missing in the
code below.

> 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: High]
Without the validation check mentioned in the commit message, if
pci->pcie_cap is uninitialized (0), will this lead to corrupting the
standard PCI configuration space header registers instead of the intended
capability registers?

[ ... ]

> 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
> @@ -806,8 +806,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  
>  	pci_host_common_link_train_delay(pci->max_link_speed);
>  
> -	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);

[Severity: High]
If pci->pcie_cap is 0, this will read from 0 + PCI_EXP_LNKSTA (0x12) instead
of the actual capability offset. Could this lead to failed link status
checks?

[ ... ]

> @@ -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 looks like it is now unused, which
might cause an unnecessary MMIO read during the capability search and trigger
a compiler warning.

>  	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
>  }
> @@ -898,7 +897,6 @@ int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
>  static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  {
>  	u32 lnkcap, lwsc, plc;
> -	u8 cap;
>  
>  	if (!num_lanes)
>  		return;
> @@ -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);
>  	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);
>  }

[Severity: High]
If pci->pcie_cap happens to be 0 here, this will perform a read-modify-write
at offset 0 + PCI_EXP_LNKCAP (0x0C). Could this destroy the standard
PCI Header Type and BIST registers?

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

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