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. The only remaining PCI_CAP_ID_EXP user is the per-function lookup in dw_pcie_ep_init_non_sticky_registers(), which uses dw_pcie_ep_find_capability() and cannot share the function 0 offset. Base branch =========== This series is based on the pci/main branch and applies on top of its current tip (see the base-commit trailer below), which is the "pci-v7.4-changes" merge. It should therefore be close to what v7.4-rc1 will contain. 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 the series does not introduce DBI accesses where the controller might not be accessible, and no driver has to call the helper at a particular time. The repeated searches are still eliminated. A DBI access performed while the controller is inaccessible is platform dependent: some integrations hang, some return zero and some return garbage. Zero self-heals (the lookup is repeated), and to make sure that a transient bogus non-zero value cannot stick, the offset is only cached after the capability ID has been read back at that offset. Otherwise the offset is used for the current access but not cached, and the next call searches again. 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 v5: - Rebase onto the pci/main branch, as suggested by Bjorn, instead of the old mainline base. - Patch 1: only cache the offset after the capability ID has been read back at that offset, so a transient bogus DBI read (for example while the controller clocks are gated) cannot be cached for the lifetime of the controller. The commit message now describes the platform dependent behavior of such accesses, as raised in the Rockchip review. - One redundant dw_pcie_get_pcie_cap() call in qcom_pcie_perst_deassert() was dropped; the L1 exit latency is programmed with the offset already fetched for the L0s one. 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]/ v4 series: https://patchwork.kernel.org/project/linux-pci/patch/[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 | 38 +++++++++++++++++++ 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, 76 insertions(+), 37 deletions(-) base-commit: 17bb0e2f6c8043f70a693a5f3797471cbed354c8 -- 2.43.0
