Re: [PATCH AUTOSEL 6.19-6.18] PCI: dwc: ep: Cache MSI outbound iATU mapping

Koichiro Den
2026-02-16T01:15:56Z

On Sat, Feb 14, 2026 at 04:23:15PM -0500, Sasha Levin wrote:
> From: Koichiro Den <den@valinux.co.jp>
>
> [ Upstream commit 8719c64e76bf258cc8f44109740c854f2e2ead2e ]
>
> dw_pcie_ep_raise_msi_irq() currently programs an outbound iATU window
> for the MSI target address on every interrupt and tears it down again
> via dw_pcie_ep_unmap_addr().
>
> On systems that heavily use the AXI bridge interface (for example when
> the integrated eDMA engine is active), this means the outbound iATU
> registers are updated while traffic is in flight. The DesignWare
> endpoint databook 5.40a - "3.10.6.1 iATU Outbound Programming Overview"
> warns that updating iATU registers in this situation is not supported,
> and the behavior is undefined.
>
> Under high MSI and eDMA load this pattern results in occasional bogus
> outbound transactions and IOMMU faults, on the RC side, such as:
>
> ipmmu-vmsa eed40000.iommu: Unhandled fault: status 0x00001502 iova 0xfe000000
>
> followed by the system becoming unresponsive. This is the actual output
> observed on Renesas R-Car S4, with its ipmmu_hc used with PCIe ch0.
>
> There is no need to reprogram the iATU region used for MSI on every
> interrupt. The host-provided MSI address is stable while MSI is enabled,
> and the endpoint driver already dedicates a scratch buffer for MSI
> generation.
>
> Cache the aligned MSI address and map size, program the outbound iATU
> once, and keep the window enabled. Subsequent interrupts only perform a
> write to the MSI scratch buffer, avoiding dynamic iATU reprogramming in
> the hot path and fixing the lockups seen under load.
>
> dw_pcie_ep_raise_msix_irq() is not modified, as each vector can have a
> different msg_addr, and because the msg_addr is allowed to be changed
> while the vector is masked. Neither problem is easy to solve with the
> current design. Instead, the plan is for the DWC vendor drivers to
> transition to dw_pcie_ep_raise_msix_irq_doorbell(), which does not rely
> on the iATU.
>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> [cassel: improve commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> Link: https://patch.msgid.link/20251222110144.3299523-2-cassel@kernel.org
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---

Hi Sasha,

Could you please hold off on backporting this patch until the follow-up
regression fix:

[PATCH] PCI: dwc: ep: Fix regression in dw_pcie_ep_raise_msi_irq()
https://lore.kernel.org/all/20260210181225.3926165-2-cassel@kernel.org/

lands upstream, so the both commits can be taken together?

The original fix ("PCI: dwc: ep: Cache MSI outbound iATU mapping") addresses an
issue that is only observable under high load, and had existed for a long time
without being reported until I posted the original fix. However, it later turned
out to introduce a regression that can lead to a system hang on some platform.

Backporting the initial commit without the follow-up fix would therefore risk
introducing a more severe problem into stable trees, at least temporarily. I
think it would be more preferable to take both commits together.

Niklas, please let me know if you would like to add anything.

Thanks,
Koichiro

>
> LLM Generated explanations, may be completely bogus:
>
> This confirms the **current state of the code** (before the patch is
> applied) still has the map/unmap on every call pattern at lines 705-712.
> The patch hasn't been applied yet to this tree — this is the candidate
> being evaluated.
>
> ### 8. STANDALONE ASSESSMENT
>
> Looking at the dependency analysis more carefully: the agent found that
> in some stable tree, this commit was brought in as part of a 10-commit
> dependency chain for a different fix. However, **that doesn't change the
> standalone value of this commit**. The commit itself:
>
> 1. **Is a standalone bug fix** — it fixes iATU reprogramming under load
> causing IOMMU faults and system lockups on real hardware
> 2. **Has no prerequisite patches** — `dw_pcie_ep_align_addr`,
> `dw_pcie_ep_map_addr`, and `dw_pcie_ep_unmap_addr` all exist in the
> current tree
> 3. **Only adds internal fields to a struct** — no API changes needed
> from other patches
> 4. **Applies cleanly** to the current code (the pre-patch code matches
> lines 704-712)
>
> ### RISK vs BENEFIT
>
> **Benefit**: HIGH
> - Fixes real system lockups under load on DWC PCIe endpoint platforms
> - The bug is documented by the hardware vendor's databook as "undefined
> behavior"
> - Concrete symptoms reported on Renesas R-Car S4
> - System becomes unresponsive — this is a critical stability issue
>
> **Risk**: LOW-MEDIUM
> - The change is well-contained within 2 files, single subsystem
> - The caching logic is straightforward and defensive (WARN_ON_ONCE on
> unexpected address change)
> - Proper cleanup on endpoint stop prevents resource leaks
> - Only affects DWC PCIe endpoint users
>
> ### CONCERNS
>
> 1. The change modifies `struct dw_pcie_ep` by adding 3 fields — this is
> safe as long as no out-of-tree modules depend on this struct layout
> (unlikely given it's a controller-specific struct).
> 2. The WARN_ON_ONCE path returns -EINVAL if MSI address changes — this
> is a safe fallback since the databook says reprogramming under load
> is undefined anyway.
> 3. The commit applies cleanly to the current tree based on code
> inspection.
>
> ### CONCLUSION
>
> This commit fixes a **real, documented hardware bug** that causes
> **system lockups** on DesignWare PCIe endpoint platforms under load. The
> fix is:
> - **Obviously correct**: Caching an iATU mapping that the hardware
> vendor's databook says should not be reprogrammed while traffic is in
> flight
> - **Well-tested**: Verified on Renesas R-Car S4
> - **Contained**: 2 files, single subsystem, no API changes
> - **Defensive**: WARN_ON_ONCE + -EINVAL on unexpected conditions, proper
> cleanup on stop
>
> This clearly meets all stable kernel criteria for a backport.
>
> **YES**
>
> .../pci/controller/dwc/pcie-designware-ep.c | 48 ++++++++++++++++---
> drivers/pci/controller/dwc/pcie-designware.h | 5 ++
> 2 files changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 19571ac2b9617..18b20cc877d9e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -601,6 +601,16 @@ static void dw_pcie_ep_stop(struct pci_epc *epc)
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>
> + /*
> + * Tear down the dedicated outbound window used for MSI
> + * generation. This avoids leaking an iATU window across
> + * endpoint stop/start cycles.
> + */
> + if (ep->msi_iatu_mapped) {
> + dw_pcie_ep_unmap_addr(epc, 0, 0, ep->msi_mem_phys);
> + ep->msi_iatu_mapped = false;
> + }
> +
> dw_pcie_stop_link(pci);
> }
>
> @@ -702,14 +712,37 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower;
>
> msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
> - ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
> - map_size);
> - if (ret)
> - return ret;
>
> - writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
> + /*
> + * Program the outbound iATU once and keep it enabled.
> + *
> + * The spec warns that updating iATU registers while there are
> + * operations in flight on the AXI bridge interface is not
> + * supported, so we avoid reprogramming the region on every MSI,
> + * specifically unmapping immediately after writel().
> + */
> + if (!ep->msi_iatu_mapped) {
> + ret = dw_pcie_ep_map_addr(epc, func_no, 0,
> + ep->msi_mem_phys, msg_addr,
> + map_size);
> + if (ret)
> + return ret;
> +
> + ep->msi_iatu_mapped = true;
> + ep->msi_msg_addr = msg_addr;
> + ep->msi_map_size = map_size;
> + } else if (WARN_ON_ONCE(ep->msi_msg_addr != msg_addr ||
> + ep->msi_map_size != map_size)) {
> + /*
> + * The host changed the MSI target address or the required
> + * mapping size changed. Reprogramming the iATU at runtime is
> + * unsafe on this controller, so bail out instead of trying to
> + * update the existing region.
> + */
> + return -EINVAL;
> + }
>
> - dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);
> + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
>
> return 0;
> }
> @@ -1087,6 +1120,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> struct device *dev = pci->dev;
>
> INIT_LIST_HEAD(&ep->func_list);
> + ep->msi_iatu_mapped = false;
> + ep->msi_msg_addr = 0;
> + ep->msi_map_size = 0;
>
> epc = devm_pci_epc_create(dev, &epc_ops);
> if (IS_ERR(epc)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 31685951a0804..f555926a526ea 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -479,6 +479,11 @@ struct dw_pcie_ep {
> void __iomem *msi_mem;
> phys_addr_t msi_mem_phys;
> struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS];
> +
> + /* MSI outbound iATU state */
> + bool msi_iatu_mapped;
> + u64 msi_msg_addr;
> + size_t msi_map_size;
> };
>
> struct dw_pcie_ops {
> --
> 2.51.0
>