Message-ID: <q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf>
Date: Sat, 31 Jan 2026 02:16:37 +0900
From: Koichiro Den <den@...inux.co.jp>
To: Niklas Cassel <cassel@...nel.org>
Cc: Aksh Garg <a-garg7@...com>, linux-pci@...r.kernel.org, 
	jingoohan1@...il.com, mani@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org, 
	robh@...nel.org, bhelgaas@...gle.com, Zhiqiang.Hou@....com, 
	gustavo.pimentel@...opsys.com, linux-kernel@...r.kernel.org, s-vadapalli@...com, 
	danishanwar@...com
Subject: Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU
 mapping support

On Fri, Jan 30, 2026 at 10:57:18AM +0100, Niklas Cassel wrote:
> On Fri, Jan 30, 2026 at 11:21:32AM +0900, Koichiro Den wrote:
> > > 
> > > Koichiro (he is on To:),
> > > 
> > > don't you think that it would be clearer if we had a:
> > > 		return;
> > > 
> > > here...
> > > 
> > > I mean, a BAR can either have a BAR match mode mapping or a subrange mapping,
> > > but not both... So continuing executing code beyond this point seems pointless,
> > > possibly even confusing.
> > 
> > Thanks for pinging. I agree it would be clearer. Since it's orthogonal to
> > this series, would it be better if I sent a one-line patch?
> 
> Thank you Koichiro, I think a one liner patch for this would be nice.
> 
> You can also write in the changelog (after the ----) that Mani can squash
> your one liner patch into the existing submap commit if he wants.

While looking at this, I spotted a subtle corner case that the missing
'return' has been masking.

Consider the following re-programming sequence for a BAR:

  1) The initial set_bar() programs the BAR in BAR Match Mode
  2) The second set_bar() switches the BAR to Address Match Mode
     - Ad part of this, set_bar() calls dw_pcie_ep_clear_ib_maps() before
       re-programming.
  3) The third set_bar() switches back to BAR Match Mode.

In step (3), the current behavior depends on how the caller handles the
struct pci_epf_bar passed to set_bar()

  a) If the caller passes a freshly prepared epf_bar with .num_submap = 0,
     dw_pcie_ep_clear_ib_maps() is called as expected.

  b) If the caller updates the same epf_bar in place (i.e. reused the
     object that passed in step (2)) and simply updates .num_submap back to
     0, dw_pcie_ep_clear_ib_maps() is NOT called because the condition in
     [1] evaluates to false. The recently merged (my) pci-epf-test code
     follows this patter, see [2].

The point is that ep_func->epf_bar[bar] only stores a pointer, and the
actual object is owned by the API caller. Since struct pci_epf embeds the
BAR descriptors, updating an epf_bar in place (pattern (b)) seems quite
natural and, in my view, should be supported. Requiring callers to always
pass a new epf_bar (pattern (a)) feels contrary to the current
API/data-structure design.

Given that, I think the cleares fix is:

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index dd48e60b513c..a20cff98b797 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -165,6 +165,9 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci
                dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index);
                clear_bit(atu_index, ep->ib_window_map);
                ep_func->bar_to_atu[bar] = 0;
+
+               WARN_ON(ep_func->ib_atu_indexes[bar]);
+               return;
        }
 
        /* Tear down all Address Match Mode mappings, if any. */
@@ -528,8 +531,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
                 * When dynamically changing a BAR, tear down any existing
                 * mappings before re-programming.
                 */
-               if (ep_func->epf_bar[bar]->num_submap || epf_bar->num_submap)
-                       dw_pcie_ep_clear_ib_maps(ep, func_no, bar);
+               dw_pcie_ep_clear_ib_maps(ep, func_no, bar);
 
                /*
                 * When dynamically changing a BAR, skip writing the BAR reg, as

---8<---

One downside is that this introduces a behavioral change in a specific
failure case that already existed prior to this series.

Concretely, this only affects the traditional reprogramming path where a
BAR configured in BAR Match Mode is updated again to BAR Match Mode. If
such an update fails (for example, because the new epf_bar->phys_addr is
not aligned to region_align), the behavior changes as follows:

  - Before this change: the previously programmed BAR Match Mode mapping
    remains in place.
  - After this change: the existing mapping is torn down first, so no mapping
    remains after the failure.

I don't think this change is a major issue, since the caller will still
observe the failure and should handle the error accordingly in either case.
That said, I'd appreciate feedback if retaining the old BAR Match Mode
mapping on such failure scenario is considered important for the existing
update path.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c?h=controller/dwc&id=0e2b9294512c140576c340b7ccd24230a593f30b#n531
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/endpoint/functions/pci-epf-test.c?h=controller/dwc&id=0e2b9294512c140576c340b7ccd24230a593f30b#n950

Kind regards,
Koichiro

> 
> 
> Kind regards,
> Niklas

Powered by blists - more mailing lists