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

[Hans Zhang <18255117159@163.com>]

On 11/7/26 03:40, Manivannan Sadhasivam wrote:
> 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.

Hello Mani,

How about modifying it to "stale the kernel documentation"? If the 
description below is valid, do I still need to send the v6 series patch?


+ * A DBI read issued while the controller is not accessible returns
+ * platform specific data, and a transient bogus value must not stick in
+ * the cache.  The offset is therefore only cached once the capability
+ * ID has been read back at that offset.  If the read-back does not
+ * confirm the capability, the offset is still returned for the current
+ * access but not cached, so the next call performs the lookup again.
+ *
+ * Return: the offset of the PCIe Capability, or 0 if it could not be
+ * found.


Best regards,
Hans


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