Re: [PATCH v5 01/16] PCI: dwc: Add pcie_cap field and caching helper

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
To: Hans Zhang <18255117159@163.com>
Cc: sashiko-reviews@lists.linux.dev, imx@lists.linux.dev,
	 linux-amlogic@lists.infradead.org, linux-pci@vger.kernel.org,
	 Frank.Li@kernel.org, neil.armstrong@linaro.org
Subject: Re: [PATCH v5 01/16] PCI: dwc: Add pcie_cap field and caching helper
Date: Sat, 07 Nov 2026 01:10:16 +0530	[thread overview]
Message-ID: <179399401605.55876.3520713514569261990.b4-reply@b4> (raw)
In-Reply-To: <3773fc58-a6ab-4f8d-a9a1-c865217afe84@163.com>

On 2026-10-28 11:18 +0800, Hans Zhang wrote:
> 
> 
> On 10/28/26 11:15, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] PCI: dwc: unconfirmed offset returned in dw_pcie_get_pcie_cap()
> > --
> > 
> > commit c6fcd840cdb57dd351ea1787cae915ad3164fb13
> > Author: Hans Zhang <18255117159@163.com>
> > 
> > PCI: dwc: Add pcie_cap field and caching helper
> > 
> > This commit adds a pcie_cap field to struct dw_pcie and introduces a
> > dw_pcie_get_pcie_cap() helper that lazily looks up and caches the PCI
> > Express Capability offset on first use once confirmed via a DBI read-back.
> > 
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >> index 6d788aa000117..dede6b3966585 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > [ ... ]
> >> @@ -832,6 +834,42 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
> >>   	dw_pcie_writel_dbi(pci, reg, val);
> >>   }
> >>   
> >> +/**
> >> + * dw_pcie_get_pcie_cap() - Return the PCIe Capability offset
> >> + * @pci: DWC instance
> >> + *
> > [ ... ]
> >> + * Return: the offset of the PCIe Capability, or 0 if it could not be
> >> + * found or has not been confirmed yet.
> >> + */
> >> +static inline u8 dw_pcie_get_pcie_cap(struct dw_pcie *pci)
> >> +{
> >> +	u8 cap = READ_ONCE(pci->pcie_cap);
> >> +
> >> +	if (!cap) {
> >> +		cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> +		if (cap && dw_pcie_readb_dbi(pci, cap) == PCI_CAP_ID_EXP)
> >> +			WRITE_ONCE(pci->pcie_cap, cap);
> >> +	}
> >> +
> >> +	return cap;
> >> +}
> > 
> > [Severity: Medium]
> > In drivers/pci/controller/dwc/pcie-designware.h, the kernel-doc for
> > dw_pcie_get_pcie_cap() states that it returns 0 if the capability could not
> > be found or has not been confirmed yet.
> > 
> > However, when pci->pcie_cap is 0 and transient DBI data causes
> > dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) to return a non-zero offset cap
> > while the confirmation check dw_pcie_readb_dbi(pci, cap) == PCI_CAP_ID_EXP
> > evaluates to false, pci->pcie_cap is left as 0, but the unconfirmed non-zero
> > cap is still returned to the caller.
> > 
> > Callers that guard DBI writes on a non-zero return (such as
> > rockchip_pcie_enable_l0s()) or cache a non-zero return in driver state (such
> > as tegra_pcie_dw_host_init() setting pcie->pcie_cap_base when
> > !pcie->pcie_cap_base) will then consume or permanently store the unconfirmed
> > offset.
> > 
> > Should dw_pcie_get_pcie_cap() return 0 when the confirmation read-back fails,
> > or should the kernel-doc comment be updated to match the implementation?
> > 
> 
> Hi,
> 
> We have already replied to Bjorn before. According to the specification, 
> the value of pcie_cap is not equal to 0.
> 

Then please update the stale the kernel doc.

- Mani

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic