Hi, The DWC PCIe core and its platform drivers repeatedly call dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) to obtain the offset of the PCI Express Capability structure, including in paths that run more than once such as link polling, suspend/resume and interrupt handlers.
Add a cached pcie_cap field to struct dw_pcie and a dw_pcie_get_pcie_cap() helper that performs the lookup on first use and caches the result, then convert the core and the 14 platform drivers that use PCI_CAP_ID_EXP. After this series, dw_pcie_find_capability(..., PCI_CAP_ID_EXP) no longer appears in drivers/pci/controller/dwc/ outside of the helper itself. Design change in v4 =================== v3 cached the offset at one chosen point (dw_pcie_host_init() after .init, or a driver's PERST# deassert handler) and then read the raw field everywhere else. As the review pointed out, that model is fragile: some paths reach the offset before the chosen caching point (the qcom .post_init callbacks, qcom-ep icc_update() from the global IRQ, the i.MX6 endpoint path) and it relies on every endpoint driver remembering to call the helper after enabling hardware. v4 turns the cache into a lazy lookup: dw_pcie_get_pcie_cap() is called at every place that needs the offset. The DBI read therefore happens at exactly the same point as the open coded dw_pcie_find_capability() call it replaces, so it is impossible to use the offset before DBI is accessible, and no driver has to call the helper at a particular time. The repeated searches are still eliminated. Sashiko review disposition ========================== As requested, here is the disposition of every non-pre-existing issue reported by Sashiko on v3: - Patch 1, "the search is repeated if the capability is not found": not a functional issue. The PCIe Capability is mandatory on DWC controllers; if it were missing, the hardware would be unusable and the only effect is that the (cheap, error path) search is repeated. This is now documented in the helper; no negative-cache flag is added. - Patch 1, data race on pci->pcie_cap: the first lookup can be triggered from more than one context, so the field is now accessed with READ_ONCE()/WRITE_ONCE(). A concurrent lookup stores the same value, hence the race is benign and no lock is needed. - Patches 2, 3, 6, 8, 16, "pci->pcie_cap may be 0 in Endpoint mode": valid for v3, fixed in v4. All users, including the endpoint ones, go through dw_pcie_get_pcie_cap(), which searches at the point of use, so a stale zero offset can no longer corrupt the standard PCI header. - Patches 2, 6, 8, "unused cap variable and redundant DBI read in dw_pcie_link_get_max_link_width()": valid, fixed in patch 2. - Patch 2, "the commit message claims a warning is printed": valid. The claim was wrong (Mani asked the same question) and has been removed; no warning is printed and none is needed, because the helper always returns the freshly searched value. - Patch 10, "icc_update() can run before the PERST# handler caches the offset": not possible in v4 and not a new issue. The lookup is done inside qcom_pcie_ep_icc_update(), exactly where the open coded search was before, so this series does not add a DBI access to the global IRQ path. The pre-existing races between the global and PERST# IRQ threads are out of scope. - Patch 11, "the qcom .post_init callbacks run before qcom_pcie_clear_aspm_l0s() populated the cache": valid for v3, fixed in v4. The .post_init callbacks now obtain the offset on their own. - Pre-existing issues (rockchip and sophgo INTx IRQ domain leak, layerscape dw_pcie_dbi_ro_wr_en()/dis() race, spear13xx phy_init() and shared IRQ handling, UltraRISC 32-bit RMW of 16-bit registers): all acknowledged, but they are not introduced by this series and are left for separate fixes. The layerscape-ep patch keeps Frank's Reviewed-by tag. The series is build tested with allmodconfig on x86_64 (drivers/pci/controller/dwc/). Changes in v4: - Rework the cache into a lazy lookup done at the point of use; drop the explicit caching calls from dw_pcie_host_init() and from the driver init/PERST# paths. This removes the whole class of "offset used before it is cached" bugs and the need for every endpoint driver to call the helper itself. - Fix the qcom .post_init ordering and the i.MX6, fu740, rockchip, UltraRISC and core endpoint paths flagged by Sashiko. - Fix the redundant lookup and the unused variable in dw_pcie_link_get_max_link_width(). - Remove the incorrect "prints a warning" statement from the core patch commit message. - Use READ_ONCE()/WRITE_ONCE() for the cached field. - Audit the whole dwc subtree: all 14 platform drivers using PCI_CAP_ID_EXP are converted, and no direct lookup remains. Changes in v3: - Remove automatic caching from dw_pcie_ep_init() to prevent unclocked DBI access on EP platforms (qcom-ep, tegra194). - Add explicit dw_pcie_get_pcie_cap() calls in qcom-ep and tegra194 EP paths after hardware enablement. - Keep Host mode caching unchanged. - Update commit messages accordingly. Changes in v2: - Move dw_pcie_get_pcie_cap() from the beginning of dw_pcie_host_init() and dw_pcie_ep_init() to after the hardware initialization callbacks (host_init/pre_init). This can prevent the reading of DBI when the clock is turned off or when the hardware is not ready. - Convert all platform drivers to use the cached offset via dw_pcie_get_pcie_cap() or directly pci->pcie_cap where safe. - Fix type mismatches (u16 -> u8). - Properly split per-driver modifications. v1 series: https://patchwork.kernel.org/project/linux-pci/cover/[email protected]/ v2 series: https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/ v3 series: https://patchwork.kernel.org/project/linux-pci/cover/[email protected]/ --- Why splitting into per-driver patches ===================================== To facilitate review and potential bisection, each platform driver is modified in its own patch. The core changes are in patches 1-2 and the follow-up patches convert individual glue drivers. If the maintainers prefer to squash them, please let me know. This splitting was carried out solely for the convenience of code review. --- Hans Zhang (16): PCI: dwc: Add pcie_cap field and caching helper PCI: dwc: Use cached PCIe capability offset in core PCI: dwc: imx6: Use cached PCIe capability offset PCI: dwc: layerscape-ep: Use cached PCIe capability offset PCI: dwc: meson: Use cached PCIe capability offset PCI: dwc: rockchip: Use cached PCIe capability offset PCI: dwc: eswin: Use cached PCIe capability offset PCI: dwc: fu740: Use cached PCIe capability offset PCI: dwc: intel-gw: Use cached PCIe capability offset PCI: dwc: qcom-ep: Use cached PCIe capability offset PCI: dwc: qcom: Use cached PCIe capability offset PCI: dwc: sophgo: Use cached PCIe capability offset PCI: dwc: spacemit-k1: Use cached PCIe capability offset PCI: dwc: spear13xx: Use cached PCIe capability offset PCI: dwc: tegra194: Use cached PCIe capability offset PCI: dwc: ultrarisc: Use cached PCIe capability offset drivers/pci/controller/dwc/pci-imx6.c | 4 +-- .../pci/controller/dwc/pci-layerscape-ep.c | 4 +-- drivers/pci/controller/dwc/pci-meson.c | 4 +-- .../pci/controller/dwc/pcie-designware-ep.c | 2 +- drivers/pci/controller/dwc/pcie-designware.c | 8 ++--- drivers/pci/controller/dwc/pcie-designware.h | 34 +++++++++++++++++++ drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +- drivers/pci/controller/dwc/pcie-eswin.c | 2 +- drivers/pci/controller/dwc/pcie-fu740.c | 2 +- drivers/pci/controller/dwc/pcie-intel-gw.c | 2 +- drivers/pci/controller/dwc/pcie-qcom-ep.c | 11 ++++---- drivers/pci/controller/dwc/pcie-qcom.c | 17 +++++----- drivers/pci/controller/dwc/pcie-sophgo.c | 5 +-- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 2 +- drivers/pci/controller/dwc/pcie-spear13xx.c | 2 +- drivers/pci/controller/dwc/pcie-tegra194.c | 6 ++-- drivers/pci/controller/dwc/pcie-ultrarisc.c | 2 +- 17 files changed, 72 insertions(+), 37 deletions(-) base-commit: 4352b8aee98005853aa63f57d6377282de17a33f -- 2.43.0
