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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In Endpoint (EP) mode, the cached `pci->pcie_cap` offset is uninitialized and defaults to 0, causing `imx_pcie_start_link` and `imx_pcie_ltssm_enable` to read and corrupt the standard PCI configuration space header instead of the PCIe Link Capabilities register.
--

--- Patch [3]: [PATCH v3 03/16] PCI: dwc: imx6: Use cached PCIe capability offset ---
commit e88cd77b027a125e9713fc310c612c5158a23be1
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: imx6: Use cached PCIe capability offset

This commit updates the i.MX6 PCIe driver to use the cached pcie_cap
offset instead of dynamically finding the PCI_CAP_ID_EXP capability
during link start and LTSSM enablement.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..74b28b4df4846 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -966,10 +966,10 @@ static void imx_pcie_ltssm_enable(struct device *dev)
>  {
>  	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
>  	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
> -	u8 offset = dw_pcie_find_capability(imx_pcie->pci, PCI_CAP_ID_EXP);
> +	struct dw_pcie *pci = imx_pcie->pci;
>  	u32 tmp;
>  
> -	tmp = dw_pcie_readl_dbi(imx_pcie->pci, offset + PCI_EXP_LNKCAP);
> +	tmp = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: High]
Is pci->pcie_cap guaranteed to be initialized here in Endpoint (EP) mode?

It looks like imx_add_pcie_ep() does not call dw_pcie_get_pcie_cap() after
the hardware is enabled. If pci->pcie_cap evaluates to 0 in EP mode, wouldn't
this read from offset 0x0C (Cache Line Size, Latency Timer, etc.) in the
standard PCI configuration space instead of the PCIe Link Capabilities register?

>  	phy_set_speed(imx_pcie->phy, FIELD_GET(PCI_EXP_LNKCAP_SLS, tmp));
>  	if (drvdata->ltssm_mask)

[ ... ]

> @@ -995,7 +995,7 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>  	struct device *dev = pci->dev;
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 tmp;
>  	int ret;

[ ... ]

>  	dw_pcie_dbi_ro_wr_en(pci);
>  	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  	tmp &= ~PCI_EXP_LNKCAP_SLS;
>  	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>  	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);

[Severity: High]
Can this corrupt the standard PCI configuration space header in EP mode?

Since offset is derived from pci->pcie_cap, if it remains uninitialized
(0) for endpoints, this write would target offset 0x0C in the standard PCI
header. This could corrupt the Cache Line Size, Latency Timer, Header Type,
and BIST registers.

Does the endpoint initialization path need a call to dw_pcie_get_pcie_cap()
before the link is started via dw_pcie_ep_start()?

>  	dw_pcie_dbi_ro_wr_dis(pci);

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