[PATCH 0/3] PCI: endpoint: Clarify pci_epc_set_bar() lifetime rules

[PATCH 0/3] PCI: endpoint: Clarify pci_epc_set_bar() lifetime rules

[Koichiro Den]

Hi,

This patch series is a follow-up to a side discussion that started from
Niklas' comment at:
https://lore.kernel.org/all/aXtrW7viGZfMNZur@ryzen/

That discussion highlighted implicit ownership/lifetime assumptions in
the pci_epc_set_bar() interface: some EPC drivers keep a reference to
the struct pci_epf_bar passed in (and its optional submap array).
Mutating or freeing those objects after a successful pci_epc_set_bar()
can lead to use-after-free or other undefined or hard-to-debug behavior.

This series:
  1) Makes BAR Match vs Address Match teardown in DesignWare EP explicit
     by returning right after clearing a BAR-match inbound mapping.
  2) Fixes pci-epf-test BAR subrange mapping to use a dedicated
     pci_epf_bar instance and to free the allocated submap only after
     restoring the default BAR mapping.
  3) Documents pci_epc_set_bar() caller ownership and lifetime rules.

Base: controller/dwc branch, latest:
commit a24149881558 ("PCI: dwc: ep: Add comment explaining controller level
                      PTM access in multi PF setup")

Thanks,

Koichiro Den (3):
  PCI: dwc: ep: Return after clearing BAR-match inbound mapping
  PCI: endpoint: pci-epf-test: Use dedicated pci_epf_bar for subrange
    mapping
  PCI: endpoint: Document pci_epc_set_bar() caller ownership and
    lifetime rules

 Documentation/PCI/endpoint/pci-endpoint.rst   | 22 +++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |  1 +
 drivers/pci/endpoint/functions/pci-epf-test.c | 32 ++++++-------------
 3 files changed, 33 insertions(+), 22 deletions(-)

-- 
2.51.0

[PATCH 1/3] PCI: dwc: ep: Return after clearing BAR-match inbound mapping

[Koichiro Den]

dw_pcie_ep_clear_ib_maps() first checks whether the inbound mapping for
a BAR is in BAR Match Mode (tracked via ep_func->bar_to_atu[bar]). Once
found, the iATU region is disabled and the bookkeeping is cleared.

BAR Match Mode and Address Match Mode mappings are mutually exclusive
for a given BAR, so there is nothing left for the Address Match Mode
teardown path to do after the BAR Match Mode mapping has been removed.

Return early after clearing the BAR Match Mode mapping to avoid running
the Address Match Mode teardown path. This makes the helper's intention
explicit and helps detect incorrect use of pci_epc_set_bar().

Link: https://lore.kernel.org/all/aXtrW7viGZfMNZur@ryzen/
Suggested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 7e7844ff0f7e..0ca05943a1e5 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -165,6 +165,7 @@ 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;
+		return;
 	}
 
 	/* Tear down all Address Match Mode mappings, if any. */
-- 
2.51.0

Re: [PATCH 1/3] PCI: dwc: ep: Return after clearing BAR-match inbound mapping

[Niklas Cassel]

On Sat, Jan 31, 2026 at 10:36:53PM +0900, Koichiro Den wrote:
> dw_pcie_ep_clear_ib_maps() first checks whether the inbound mapping for
> a BAR is in BAR Match Mode (tracked via ep_func->bar_to_atu[bar]). Once
> found, the iATU region is disabled and the bookkeeping is cleared.
> 
> BAR Match Mode and Address Match Mode mappings are mutually exclusive
> for a given BAR, so there is nothing left for the Address Match Mode
> teardown path to do after the BAR Match Mode mapping has been removed.
> 
> Return early after clearing the BAR Match Mode mapping to avoid running
> the Address Match Mode teardown path. This makes the helper's intention
> explicit and helps detect incorrect use of pci_epc_set_bar().
> 
> Link: https://lore.kernel.org/all/aXtrW7viGZfMNZur@ryzen/
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH 2/3] PCI: endpoint: pci-epf-test: Use dedicated pci_epf_bar for subrange mapping

[Koichiro Den]

The BAR subrange setup/clear paths in pci-epf-test used to update
epf->bar[barno].submap in place and free/restore the submap around
pci_epc_set_bar() calls.

Some EPC drivers may keep a reference to the struct pci_epf_bar passed
to pci_epc_set_bar(). Mutating or freeing the same bar descriptor after
a successful set_bar() can therefore lead to unexpected behaviour.

Use a dedicated pci_epf_bar instance for the subrange mapping test and
only free the allocated submap after restoring the BAR mapping back to
the default epf->bar[barno] descriptor.

Fixes: 6c5e6101423b ("PCI: endpoint: pci-epf-test: Add BAR subrange mapping test support")
Suggested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 32 ++++++-------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 6952ee418622..fd6452d1dcc7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -86,6 +86,7 @@ struct pci_epf_test {
 	bool			dma_private;
 	const struct pci_epc_features *epc_features;
 	struct pci_epf_bar	db_bar;
+	struct pci_epf_bar	subrange_bar[PCI_STD_NUM_BARS];
 	size_t			bar_size[PCI_STD_NUM_BARS];
 };
 
@@ -827,11 +828,11 @@ static u8 pci_epf_test_subrange_sig_byte(enum pci_barno barno,
 static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 					    struct pci_epf_test_reg *reg)
 {
-	struct pci_epf_bar_submap *submap, *old_submap;
+	struct pci_epf_bar_submap *submap;
 	struct pci_epf *epf = epf_test->epf;
 	struct pci_epc *epc = epf->epc;
 	struct pci_epf_bar *bar;
-	unsigned int nsub = PCI_EPF_TEST_BAR_SUBRANGE_NSUB, old_nsub;
+	unsigned int nsub = PCI_EPF_TEST_BAR_SUBRANGE_NSUB;
 	/* reg->size carries BAR number for BAR_SUBRANGE_* commands. */
 	enum pci_barno barno = le32_to_cpu(reg->size);
 	u32 status = le32_to_cpu(reg->status);
@@ -857,7 +858,8 @@ static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 		goto err;
 	}
 
-	bar = &epf->bar[barno];
+	bar = &epf_test->subrange_bar[barno];
+	*bar = epf->bar[barno];
 	if (!bar->size || !bar->addr) {
 		dev_err(&epf->dev, "bar size/addr (%zu/%p) is invalid\n",
 			bar->size, bar->addr);
@@ -883,21 +885,17 @@ static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 		submap[i].size = sub_size;
 	}
 
-	old_submap = bar->submap;
-	old_nsub = bar->num_submap;
-
 	bar->submap = submap;
 	bar->num_submap = nsub;
 
 	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, bar);
 	if (ret) {
 		dev_err(&epf->dev, "pci_epc_set_bar() failed: %d\n", ret);
-		bar->submap = old_submap;
-		bar->num_submap = old_nsub;
+		bar->submap = NULL;
+		bar->num_submap = 0;
 		kfree(submap);
 		goto err;
 	}
-	kfree(old_submap);
 
 	/*
 	 * Fill deterministic signatures into the physical regions that
@@ -925,13 +923,11 @@ static void pci_epf_test_bar_subrange_clear(struct pci_epf_test *epf_test,
 					    struct pci_epf_test_reg *reg)
 {
 	struct pci_epf *epf = epf_test->epf;
-	struct pci_epf_bar_submap *submap;
 	struct pci_epc *epc = epf->epc;
 	/* reg->size carries BAR number for BAR_SUBRANGE_* commands. */
 	enum pci_barno barno = le32_to_cpu(reg->size);
 	u32 status = le32_to_cpu(reg->status);
 	struct pci_epf_bar *bar;
-	unsigned int nsub;
 	int ret;
 
 	if (barno >= PCI_STD_NUM_BARS) {
@@ -940,23 +936,15 @@ static void pci_epf_test_bar_subrange_clear(struct pci_epf_test *epf_test,
 	}
 
 	bar = &epf->bar[barno];
-	submap = bar->submap;
-	nsub = bar->num_submap;
-
-	if (!submap || !nsub)
-		goto err;
-
-	bar->submap = NULL;
-	bar->num_submap = 0;
 
 	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, bar);
 	if (ret) {
-		bar->submap = submap;
-		bar->num_submap = nsub;
 		dev_err(&epf->dev, "pci_epc_set_bar() failed: %d\n", ret);
 		goto err;
 	}
-	kfree(submap);
+	kfree(epf_test->subrange_bar[barno].submap);
+	epf_test->subrange_bar[barno].submap = NULL;
+	epf_test->subrange_bar[barno].num_submap = 0;
 
 	status |= STATUS_BAR_SUBRANGE_CLEAR_SUCCESS;
 	reg->status = cpu_to_le32(status);
-- 
2.51.0

Re: [PATCH 2/3] PCI: endpoint: pci-epf-test: Use dedicated pci_epf_bar for subrange mapping

[Niklas Cassel]

On Sat, Jan 31, 2026 at 10:36:54PM +0900, Koichiro Den wrote:
> The BAR subrange setup/clear paths in pci-epf-test used to update
> epf->bar[barno].submap in place and free/restore the submap around
> pci_epc_set_bar() calls.
>
> Some EPC drivers may keep a reference to the struct pci_epf_bar passed
> to pci_epc_set_bar(). Mutating or freeing the same bar descriptor after
> a successful set_bar() can therefore lead to unexpected behaviour.
>
> Use a dedicated pci_epf_bar instance for the subrange mapping test and
> only free the allocated submap after restoring the BAR mapping back to
> the default epf->bar[barno] descriptor.
>
> Fixes: 6c5e6101423b ("PCI: endpoint: pci-epf-test: Add BAR subrange mapping test support")
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 32 ++++++-------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 6952ee418622..fd6452d1dcc7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -86,6 +86,7 @@ struct pci_epf_test {
>	bool			dma_private;
>	const struct pci_epc_features *epc_features;
>	struct pci_epf_bar	db_bar;
> +	struct pci_epf_bar	subrange_bar[PCI_STD_NUM_BARS];


If we compare your test:
pci_epf_test_bar_subrange_setup(), the host side decides which BAR you
want to configure.

For pci_epf_test_enable_doorbell(), the function itself uses
pci_epc_get_next_free_bar(), so the EP side decides which BAR to use.

This is a difference, but I think your way is fine.



Another difference is that you have:
struct pci_epf_bar      subrange_bar[PCI_STD_NUM_BARS];

while the doorbell test case has:

struct pci_epf_bar      db_bar;

Looking at the code, you allow multiple BARs to be configured in subrange
mapping mode (even though the selftest itself will only enable+disable it
for one BAR one by one, but I guess someone could theoretically write their
own test program that puts all the BARs in subrange mapping mode at the same
time).

This is another difference from enable_doorbell(), but again I think your
way is also fine.



Looking at the pci-epf-test code, I realize that, because:
struct pci_epf_bar      db_bar;
is just a single struct, doing ioctl ENABLE_DOORBELL multiple times will
just overwrite the existing db_bar struct... Not very nice...

Since it is only one db_bar, pci-epf-test should return an error if
ENABLE_DOORBELL is called multiple times in a row, rather than just silently
overwrite pci_epf_bar db_bar, leaving the previous BAR still configured
while programming yet another BAR for a HW doorbell... This means that
calling DISABLE_DOORBELL will incorrectly just cleanup one BAR and not two...
This is not your bug however...



TL;DR: I think your code looks fine, even though it is different that
the doorbell test case in a few ways.

Also the doorbell test case is buggy, but that is not really your problem.


Kind regards,
Niklas

[PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Koichiro Den]

pci_epc_set_bar() may be called multiple times for a BAR when an
endpoint controller supports dynamic_inbound_mapping and/or
subrange_mapping.

Some EPC drivers keep a reference to the struct pci_epf_bar passed to
pci_epc_set_bar(), but the documentation does not describe the ownership
and lifetime rules for that object (and its submap array).

Document that the EPF driver retains ownership of these objects, must
keep them valid, and must not modify them after a successful
pci_epc_set_bar(). When updating an active mapping, the EPF driver must
pass a new pci_epf_bar instance and only free the old one after the
update succeeds.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 Documentation/PCI/endpoint/pci-endpoint.rst | 22 +++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
index 4697377adeae..b2f5ad147ed8 100644
--- a/Documentation/PCI/endpoint/pci-endpoint.rst
+++ b/Documentation/PCI/endpoint/pci-endpoint.rst
@@ -119,6 +119,28 @@ by the PCI endpoint function driver.
    BAR register or BAR decode on the endpoint while the host still expects
    the assigned BAR address to remain valid.
 
+   The struct pci_epf_bar passed to pci_epc_set_bar() (and the optional
+   pci_epf_bar.submap array) is owned by the PCI endpoint function driver.
+   An EPC driver may keep a reference to these objects after
+   pci_epc_set_bar() returns. Therefore the EPF driver must ensure that:
+
+     * Ownership of the pci_epf_bar object passed to pci_epc_set_bar()
+       remains with the caller (the EPF driver). The caller is responsible
+       for ensuring it remains valid (and freeing it when dynamically
+       allocated).
+
+     * After pci_epc_set_bar() succeeds, the caller must not modify the
+       contents of the pci_epf_bar object (or its submap array) until a
+       later successful pci_epc_set_bar() for the same BAR replaces it, or
+       until pci_epc_clear_bar() succeeds. Otherwise, it could potentially
+       lead to use-after-free or undefined behavior.
+
+     * If the caller needs to update the mapping for a BAR and calls
+       pci_epc_set_bar() again, it should use a new pci_epf_bar instance
+       (and a new submap array, if used). If the call succeeds, the old
+       instance can then be freed by the caller. If the call fails, the old
+       instance must remain valid.
+
 * pci_epc_clear_bar()
 
    The PCI endpoint function driver should use pci_epc_clear_bar() to reset
-- 
2.51.0

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Niklas Cassel]

On Sat, Jan 31, 2026 at 10:36:55PM +0900, Koichiro Den wrote:
> pci_epc_set_bar() may be called multiple times for a BAR when an
> endpoint controller supports dynamic_inbound_mapping and/or
> subrange_mapping.
> 
> Some EPC drivers keep a reference to the struct pci_epf_bar passed to
> pci_epc_set_bar(), but the documentation does not describe the ownership
> and lifetime rules for that object (and its submap array).
> 
> Document that the EPF driver retains ownership of these objects, must
> keep them valid, and must not modify them after a successful
> pci_epc_set_bar(). When updating an active mapping, the EPF driver must
> pass a new pci_epf_bar instance and only free the old one after the
> update succeeds.
> 
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  Documentation/PCI/endpoint/pci-endpoint.rst | 22 +++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
> index 4697377adeae..b2f5ad147ed8 100644
> --- a/Documentation/PCI/endpoint/pci-endpoint.rst
> +++ b/Documentation/PCI/endpoint/pci-endpoint.rst
> @@ -119,6 +119,28 @@ by the PCI endpoint function driver.
>     BAR register or BAR decode on the endpoint while the host still expects
>     the assigned BAR address to remain valid.
>  
> +   The struct pci_epf_bar passed to pci_epc_set_bar() (and the optional
> +   pci_epf_bar.submap array) is owned by the PCI endpoint function driver.
> +   An EPC driver may keep a reference to these objects after
> +   pci_epc_set_bar() returns. Therefore the EPF driver must ensure that:
> +
> +     * Ownership of the pci_epf_bar object passed to pci_epc_set_bar()
> +       remains with the caller (the EPF driver). The caller is responsible
> +       for ensuring it remains valid (and freeing it when dynamically
> +       allocated).
> +
> +     * After pci_epc_set_bar() succeeds, the caller must not modify the
> +       contents of the pci_epf_bar object (or its submap array) until a
> +       later successful pci_epc_set_bar() for the same BAR replaces it, or
> +       until pci_epc_clear_bar() succeeds. Otherwise, it could potentially
> +       lead to use-after-free or undefined behavior.
> +
> +     * If the caller needs to update the mapping for a BAR and calls
> +       pci_epc_set_bar() again, it should use a new pci_epf_bar instance
> +       (and a new submap array, if used). If the call succeeds, the old

Why does it need a new submap array?

Since an EPC driver never frees the pci_epf_bar instance, nor never frees
the submap array, an EPF driver could reuse the submap in two consecutive
set_bar() if it so wanted, even though it would be a bit silly.

I guess my point is that the important thing is that the pci_epf_bar and
the submap is immutable / pointer to const from EPC's point of view.

Since the EPC will not change the pci_epf_bar, EPF driver could also
theoretically call set_bar() twice with the exact same pci_epf_bar,
even though that would be a bit silly.


IMO, we could totally avoid all this text if we just changed;
int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
                    struct pci_epf_bar *epf_bar);

to:
int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
                    const struct pci_epf_bar * const epf_bar);


i.e.  const pointer to something const, because set_bar() will not change
the pointer, and what is being passed should not change.


Note that I'm not asking you to do this change in all the drivers,
I'm just saying that if the API was actually defined like this,
we would not need to add any Documentation, because the code would
speak for itself.


I think this patch is good, if we just rephrase it slightly.
(An EPF driver can send in the same struct bar twice, it just can't
modify the current struct bar while it is "in use".)
We can probably write this in two paragraphs instead of three.


Kind regards,
Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Koichiro Den]

On Sat, Jan 31, 2026 at 05:50:38PM +0100, Niklas Cassel wrote:
> On Sat, Jan 31, 2026 at 10:36:55PM +0900, Koichiro Den wrote:
> > pci_epc_set_bar() may be called multiple times for a BAR when an
> > endpoint controller supports dynamic_inbound_mapping and/or
> > subrange_mapping.
> > 
> > Some EPC drivers keep a reference to the struct pci_epf_bar passed to
> > pci_epc_set_bar(), but the documentation does not describe the ownership
> > and lifetime rules for that object (and its submap array).
> > 
> > Document that the EPF driver retains ownership of these objects, must
> > keep them valid, and must not modify them after a successful
> > pci_epc_set_bar(). When updating an active mapping, the EPF driver must
> > pass a new pci_epf_bar instance and only free the old one after the
> > update succeeds.
> > 
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> >  Documentation/PCI/endpoint/pci-endpoint.rst | 22 +++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
> > index 4697377adeae..b2f5ad147ed8 100644
> > --- a/Documentation/PCI/endpoint/pci-endpoint.rst
> > +++ b/Documentation/PCI/endpoint/pci-endpoint.rst
> > @@ -119,6 +119,28 @@ by the PCI endpoint function driver.
> >     BAR register or BAR decode on the endpoint while the host still expects
> >     the assigned BAR address to remain valid.
> >  
> > +   The struct pci_epf_bar passed to pci_epc_set_bar() (and the optional
> > +   pci_epf_bar.submap array) is owned by the PCI endpoint function driver.
> > +   An EPC driver may keep a reference to these objects after
> > +   pci_epc_set_bar() returns. Therefore the EPF driver must ensure that:
> > +
> > +     * Ownership of the pci_epf_bar object passed to pci_epc_set_bar()
> > +       remains with the caller (the EPF driver). The caller is responsible
> > +       for ensuring it remains valid (and freeing it when dynamically
> > +       allocated).
> > +
> > +     * After pci_epc_set_bar() succeeds, the caller must not modify the
> > +       contents of the pci_epf_bar object (or its submap array) until a
> > +       later successful pci_epc_set_bar() for the same BAR replaces it, or
> > +       until pci_epc_clear_bar() succeeds. Otherwise, it could potentially
> > +       lead to use-after-free or undefined behavior.
> > +
> > +     * If the caller needs to update the mapping for a BAR and calls
> > +       pci_epc_set_bar() again, it should use a new pci_epf_bar instance
> > +       (and a new submap array, if used). If the call succeeds, the old
> 
> Why does it need a new submap array?
> 
> Since an EPC driver never frees the pci_epf_bar instance, nor never frees
> the submap array, an EPF driver could reuse the submap in two consecutive
> set_bar() if it so wanted, even though it would be a bit silly.

You're right, the phrase "(and a new submap array, if used)" was
overreaching. The .submap does not necessarily need to be a new
allocation.

> 
> I guess my point is that the important thing is that the pci_epf_bar and
> the submap is immutable / pointer to const from EPC's point of view.
> 
> Since the EPC will not change the pci_epf_bar, EPF driver could also
> theoretically call set_bar() twice with the exact same pci_epf_bar,
> even though that would be a bit silly.
> 
> 
> IMO, we could totally avoid all this text if we just changed;
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>                     struct pci_epf_bar *epf_bar);
> 
> to:
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>                     const struct pci_epf_bar * const epf_bar);
> 
> 
> i.e.  const pointer to something const, because set_bar() will not change
> the pointer, and what is being passed should not change.
> 
> 
> Note that I'm not asking you to do this change in all the drivers,
> I'm just saying that if the API was actually defined like this,
> we would not need to add any Documentation, because the code would
> speak for itself.

I agree that the const-ification would be useful to some extent. That makes
it explicit that the EPC must not modify the epf_bar.

However, I don't think it removes the need for the documentation, because
the added text is more about ownership, lifetime and about what the caller
must (and must not) do after set_bar() returns.

Even without the third bullet point ("If the caller needs to update ..."),
const-ifying the function parameter does not enforce the first and second
points either. The caller can still keep its own non-const reference and
mutate the same object after a successful pci_epc_set_bar().

For example, epf-vntb initializes BARs via pci_epc_set_bar() with phys_addr
= 0 [1], and later updates the same ntb->epf->bar[barno] fields in-place
and calls pci_epc_set_bar() again [2] via the .mw_set_trans callback:

  [1] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L710
  [2] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L1288

So, if [PATCH 3/3] is the contract we can agree on, then I think epf-vntb
would likely need an adjustment to follow that contract (i.e. avoid
mutating the descriptor that might still be referenced by the EPC, and
instead switch to a different instance when updating). In that sense, the
code alone seems not always to speak for itself at the moment, and having
the agreement documented would still be valuable for future EPF
implementations.

Kind regards,
Koichiro

> 
> 
> I think this patch is good, if we just rephrase it slightly.
> (An EPF driver can send in the same struct bar twice, it just can't
> modify the current struct bar while it is "in use".)
> We can probably write this in two paragraphs instead of three.
> 
> 
> Kind regards,
> Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Niklas Cassel]

On Mon, Feb 02, 2026 at 12:45:52AM +0900, Koichiro Den wrote:
> For example, epf-vntb initializes BARs via pci_epc_set_bar() with phys_addr
> = 0 [1], and later updates the same ntb->epf->bar[barno] fields in-place
> and calls pci_epc_set_bar() again [2] via the .mw_set_trans callback:
> 
>   [1] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L710
>   [2] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L1288
> 
> So, if [PATCH 3/3] is the contract we can agree on, then I think epf-vntb
> would likely need an adjustment to follow that contract (i.e. avoid
> mutating the descriptor that might still be referenced by the EPC, and
> instead switch to a different instance when updating). In that sense, the
> code alone seems not always to speak for itself at the moment, and having
> the agreement documented would still be valuable for future EPF
> implementations.

You are absolutely right!


The pci-epf-test code that uses a difference instance was made by:

commit eff0c286aa916221a69126a43eee7c218d6f4011
Author: Frank Li <Frank.Li@nxp.com>
Date:   Thu Jul 10 15:13:52 2025 -0400

    PCI: endpoint: pci-epf-test: Add doorbell test support


The pci-epf-vntb code that uses the same instance was made by:

commit e35f56bb03304abc92c928b641af41ca372966bb
Author: Frank Li <Frank.Li@nxp.com>
Date:   Tue Feb 22 10:23:54 2022 -0600

    PCI: endpoint: Support NTB transfer between RC and EP



The dynamically update inbound address support in the DWC driver itself
was made by:

commit 4284c88fff0efc4e418abb53d78e02dc4f099d6c
Author: Frank Li <Frank.Li@nxp.com>
Date:   Tue Feb 22 10:23:52 2022 -0600

    PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address



I don't know why Frank chose to use the same API in two different ways.
Perhaps because in pci-epf-test.c he needed to be able to restore the
BAR to the original state when calling DISABLE_DOORBELL, but in vntb
there was no need to "restore" to the original state.


So, perhaps we should allow in place updates after all...
Frank, thoughts?


Considering that struct pci_epf_bar lives in struct pci_epf, I think my
previous idea of doing a kmemdup, seems wrong...

I think you are right that in place updates do make sense in one way...
even if it can mean that the current safe guards can by bypassed..

However, if you modify the struct pci_epf_bar in an incompatible way,
before calling set_bar() or clear_bar(), it is your own fault...


Considering that pci-epf-vntb does in place updates... we probably should
allow in place updates as well... As you suggested a few days ago:
https://lore.kernel.org/linux-pci/q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf/

I'm sorry for changing my mind. I did not know that there were any
EPF driver that already did in place updates...

I did look at how pci-epf-vntb handled doorbells, but it called either:
epf_ntb_db_bar_init_msi_doorbell() or uses polling, but in either case
it never seemed to call set_bar() twice (at least not to set the doorbell),
so as far as saw, it did not do in place updates.

Considering that we probably want to support in place updates after all...

I guess we probably only need patch 1/3 in this series, plus another
patch that makes sure that we call dw_pcie_ep_clear_ib_maps() unconditionally?

I still don't like that dw_pcie_ep_clear_ib_maps() will be called
unconditionally, but I don't see any other way to support in place updates...

I'm sorry for making you waste time. I did miss that even though pci-epf-vntb
does not do in place updates of doorbell BAR, it does so for the other BARs.


Kind regards,
Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Niklas Cassel]

On Sun, Feb 01, 2026 at 10:37:08PM +0100, Niklas Cassel wrote:
> 
> Considering that we probably want to support in place updates after all...
> 
> I guess we probably only need patch 1/3 in this series, plus another
> patch that makes sure that we call dw_pcie_ep_clear_ib_maps() unconditionally?
> 
> I still don't like that dw_pcie_ep_clear_ib_maps() will be called
> unconditionally, but I don't see any other way to support in place updates...

Perhaps just add a comment above the unconditonal call to
dw_pcie_ep_clear_ib_maps() which explains why the call is unconditional
(i.e. it has to be unconditional in order to support in place updates).


Kind regards,
Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Koichiro Den]

On Sun, Feb 01, 2026 at 10:37:02PM +0100, Niklas Cassel wrote:
> On Mon, Feb 02, 2026 at 12:45:52AM +0900, Koichiro Den wrote:
> > For example, epf-vntb initializes BARs via pci_epc_set_bar() with phys_addr
> > = 0 [1], and later updates the same ntb->epf->bar[barno] fields in-place
> > and calls pci_epc_set_bar() again [2] via the .mw_set_trans callback:
> > 
> >   [1] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L710
> >   [2] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/endpoint/functions/pci-epf-vntb.c#L1288
> > 
> > So, if [PATCH 3/3] is the contract we can agree on, then I think epf-vntb
> > would likely need an adjustment to follow that contract (i.e. avoid
> > mutating the descriptor that might still be referenced by the EPC, and
> > instead switch to a different instance when updating). In that sense, the
> > code alone seems not always to speak for itself at the moment, and having
> > the agreement documented would still be valuable for future EPF
> > implementations.
> 
> You are absolutely right!
> 
> 
> The pci-epf-test code that uses a difference instance was made by:
> 
> commit eff0c286aa916221a69126a43eee7c218d6f4011
> Author: Frank Li <Frank.Li@nxp.com>
> Date:   Thu Jul 10 15:13:52 2025 -0400
> 
>     PCI: endpoint: pci-epf-test: Add doorbell test support
> 
> 
> The pci-epf-vntb code that uses the same instance was made by:
> 
> commit e35f56bb03304abc92c928b641af41ca372966bb
> Author: Frank Li <Frank.Li@nxp.com>
> Date:   Tue Feb 22 10:23:54 2022 -0600
> 
>     PCI: endpoint: Support NTB transfer between RC and EP
> 
> 
> 
> The dynamically update inbound address support in the DWC driver itself
> was made by:
> 
> commit 4284c88fff0efc4e418abb53d78e02dc4f099d6c
> Author: Frank Li <Frank.Li@nxp.com>
> Date:   Tue Feb 22 10:23:52 2022 -0600
> 
>     PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address
> 
> 
> 
> I don't know why Frank chose to use the same API in two different ways.
> Perhaps because in pci-epf-test.c he needed to be able to restore the
> BAR to the original state when calling DISABLE_DOORBELL, but in vntb
> there was no need to "restore" to the original state.
> 
> 
> So, perhaps we should allow in place updates after all...
> Frank, thoughts?
> 
> 
> Considering that struct pci_epf_bar lives in struct pci_epf, I think my
> previous idea of doing a kmemdup, seems wrong...
> 

I don't think it's inherently wrong. I think it really comes down to what
contract we want pci_epc_set_bar() to imply.

When I saw your earlier comment:
https://lore.kernel.org/all/aX019VTWjMlPX8qp@fedora/
I hastily assumed you were implicitly suggesting that there are some
outliers (such as epf-vntb), which led me to think we should document a
single "legit" way to use the API. In hindsight, I read too much into it,
there doesn't seem to be a clearly established contract today.

One subtlety if we decide to treat in-place updates as supported: the
existing dynamic update compatibility check in dwc [3] becomes officially
best-effort, because ep->epf_bar[bar] and the passed-in epf_bar may point
to the same object (so comparing against the previous state is not
reliable). In other words, changing barno/size/flags via in-place updates
would be caller misuse, but the driver cannot always detect it.

  [3] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L368

That said, this seems to be already effectively best-effort on mainline
today where in-place updates are being done, so I don't think we need to
rush fixing this part immediately.

> I think you are right that in place updates do make sense in one way...
> even if it can mean that the current safe guards can by bypassed..
> 
> However, if you modify the struct pci_epf_bar in an incompatible way,
> before calling set_bar() or clear_bar(), it is your own fault...
> 
> 
> Considering that pci-epf-vntb does in place updates... we probably should
> allow in place updates as well... As you suggested a few days ago:
> https://lore.kernel.org/linux-pci/q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf/
> 
> I'm sorry for changing my mind. I did not know that there were any
> EPF driver that already did in place updates...
> 
> I did look at how pci-epf-vntb handled doorbells, but it called either:
> epf_ntb_db_bar_init_msi_doorbell() or uses polling, but in either case
> it never seemed to call set_bar() twice (at least not to set the doorbell),
> so as far as saw, it did not do in place updates.
> 
> Considering that we probably want to support in place updates after all...
> 
> I guess we probably only need patch 1/3 in this series, plus another
> patch that makes sure that we call dw_pcie_ep_clear_ib_maps() unconditionally?
> 
> I still don't like that dw_pcie_ep_clear_ib_maps() will be called
> unconditionally, but I don't see any other way to support in place updates...
> 
> I'm sorry for making you waste time. I did miss that even though pci-epf-vntb
> does not do in place updates of doorbell BAR, it does so for the other BARs.

No worries at all, and thanks for digging through the history with me.
At this point, I think there are still two reasonable options (to
summarize):

X). Treat the existing in-tree callers (including in-place update) as valid
    usage (i.e. apply [4]).

    [4] https://lore.kernel.org/linux-pci/q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf/

    In this case, the downside noted in [4] remains: if a BAR reprogramming
    attempt fails (especially for the long-standing epf-vntb's BAR Match ->
    BAR Match transition case), the previously programmed inbound mapping
    will already have been torn down. This behavior change is inherent in
    making the teardown unconditional. I think this is acceptable because
    if the caller is passing incompatible/invalid parameters, things are
    already going off the rails anyway, and the call site that receives the
    error should never actively use the BAR for any real transactions.

    Separately, if we treat in-place updates as supported, some of the
    existing compatibility checks (e.g. barno/size/flags) become inherently
    best-effort, because the previous state may no longer be observable by
    the driver. Addressing that would require additional follow-up work
    (e.g. with doing a kmemdup and holding the snapshot), but this is a
    pre-existing issue, so there is no need to rush fixing this.

Y). Define a stricter API usage contract, document it, and then adjust all
    the caller sides later (i.e. apply this v2 series).

    The downside here is that struct pci_epf embeds the struct pci_epf_bar
    array, so tightening the contract and fixing existing users would
    likely be awkward.

Personally, I'm inclined towards (X) at the moment, mainly because there
doesn't seem to be a firm, shared understanding of the API contract today.
Later, we can do follow-up work for the existing behaviour, which is
already present on mainline.

If you still agree with (X), I'll send v2 with splitting [4] into two-patch
series, with an explanation above the unconditional
dw_pcie_ep_clear_ib_maps().

Thank you again for the time and the discussion,
Koichiro

> 
> 
> Kind regards,
> Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Niklas Cassel]

On Mon, Feb 02, 2026 at 02:59:35PM +0900, Koichiro Den wrote:
> > 
> > Considering that struct pci_epf_bar lives in struct pci_epf, I think my
> > previous idea of doing a kmemdup, seems wrong...
> > 
> 
> I don't think it's inherently wrong. I think it really comes down to what
> contract we want pci_epc_set_bar() to imply.
> 
> When I saw your earlier comment:
> https://lore.kernel.org/all/aX019VTWjMlPX8qp@fedora/
> I hastily assumed you were implicitly suggesting that there are some
> outliers (such as epf-vntb), which led me to think we should document a
> single "legit" way to use the API. In hindsight, I read too much into it,
> there doesn't seem to be a clearly established contract today.
> 
> One subtlety if we decide to treat in-place updates as supported: the
> existing dynamic update compatibility check in dwc [3] becomes officially
> best-effort, because ep->epf_bar[bar] and the passed-in epf_bar may point
> to the same object (so comparing against the previous state is not
> reliable). In other words, changing barno/size/flags via in-place updates
> would be caller misuse, but the driver cannot always detect it.

Yes, I agree, but I think that is fine.

If the caller does a fundamental change to an existing struct pci_epf_bar,
between two set_bar() calls... they have no one to blame but themselves.

At least the check will be able to detect when the second set_bar() call
is supplied a new struct which does not have the same size / flags as the
struct pci_epf_bar that is currently in use.

The same currently applies to clear_bar():
If you do a stupid in place update of the struct pci_epf_bar after calling
set_bar(), e.g. modifying epf_bar->barno, clear_bar() will absolutely do
"bad things".

Perhaps we should update the comment in dw_pcie_ep_set_bar():

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 7e7844ff0f7e..451ba8add157 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -518,6 +518,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
                /*
                 * We can only dynamically change a BAR if the new BAR size and
                 * BAR flags do not differ from the existing configuration.
+                *
+                * Note: this safety check only works when the caller uses a new
+                * struct pci_epf_bar in the second set_bar() call. If the same
+                * struct pci_epf_bar was supplied (i.e. being updated in place)
+                * then it is impossible to detect invalid changes to the BAR.
                 */
                if (ep_func->epf_bar[bar]->barno != bar ||
                    ep_func->epf_bar[bar]->size != size ||


To make it clear that this safety check is not always possible.


> > I'm sorry for making you waste time. I did miss that even though pci-epf-vntb
> > does not do in place updates of doorbell BAR, it does so for the other BARs.
> 
> No worries at all, and thanks for digging through the history with me.
> At this point, I think there are still two reasonable options (to
> summarize):
> 
> X). Treat the existing in-tree callers (including in-place update) as valid
>     usage (i.e. apply [4]).
> 
>     [4] https://lore.kernel.org/linux-pci/q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf/
> 
>     In this case, the downside noted in [4] remains: if a BAR reprogramming
>     attempt fails (especially for the long-standing epf-vntb's BAR Match ->
>     BAR Match transition case), the previously programmed inbound mapping
>     will already have been torn down. This behavior change is inherent in
>     making the teardown unconditional. I think this is acceptable because
>     if the caller is passing incompatible/invalid parameters, things are
>     already going off the rails anyway, and the call site that receives the
>     error should never actively use the BAR for any real transactions.
> 
>     Separately, if we treat in-place updates as supported, some of the
>     existing compatibility checks (e.g. barno/size/flags) become inherently
>     best-effort, because the previous state may no longer be observable by
>     the driver. Addressing that would require additional follow-up work
>     (e.g. with doing a kmemdup and holding the snapshot), but this is a
>     pre-existing issue, so there is no need to rush fixing this.
> 
> Y). Define a stricter API usage contract, document it, and then adjust all
>     the caller sides later (i.e. apply this v2 series).
> 
>     The downside here is that struct pci_epf embeds the struct pci_epf_bar
>     array, so tightening the contract and fixing existing users would
>     likely be awkward.
> 
> Personally, I'm inclined towards (X) at the moment, mainly because there
> doesn't seem to be a firm, shared understanding of the API contract today.
> Later, we can do follow-up work for the existing behaviour, which is
> already present on mainline.
> 
> If you still agree with (X), I'll send v2 with splitting [4] into two-patch
> series, with an explanation above the unconditional
> dw_pcie_ep_clear_ib_maps().

I did not change my mind a second time :)

So I still think X is the way to go.


Kind regards,
Niklas

Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller ownership and lifetime rules

[Koichiro Den]

On Mon, Feb 02, 2026 at 10:27:12AM +0100, Niklas Cassel wrote:
> On Mon, Feb 02, 2026 at 02:59:35PM +0900, Koichiro Den wrote:
> > > 
> > > Considering that struct pci_epf_bar lives in struct pci_epf, I think my
> > > previous idea of doing a kmemdup, seems wrong...
> > > 
> > 
> > I don't think it's inherently wrong. I think it really comes down to what
> > contract we want pci_epc_set_bar() to imply.
> > 
> > When I saw your earlier comment:
> > https://lore.kernel.org/all/aX019VTWjMlPX8qp@fedora/
> > I hastily assumed you were implicitly suggesting that there are some
> > outliers (such as epf-vntb), which led me to think we should document a
> > single "legit" way to use the API. In hindsight, I read too much into it,
> > there doesn't seem to be a clearly established contract today.
> > 
> > One subtlety if we decide to treat in-place updates as supported: the
> > existing dynamic update compatibility check in dwc [3] becomes officially
> > best-effort, because ep->epf_bar[bar] and the passed-in epf_bar may point
> > to the same object (so comparing against the previous state is not
> > reliable). In other words, changing barno/size/flags via in-place updates
> > would be caller misuse, but the driver cannot always detect it.
> 
> Yes, I agree, but I think that is fine.
> 
> If the caller does a fundamental change to an existing struct pci_epf_bar,
> between two set_bar() calls... they have no one to blame but themselves.
> 
> At least the check will be able to detect when the second set_bar() call
> is supplied a new struct which does not have the same size / flags as the
> struct pci_epf_bar that is currently in use.
> 
> The same currently applies to clear_bar():
> If you do a stupid in place update of the struct pci_epf_bar after calling
> set_bar(), e.g. modifying epf_bar->barno, clear_bar() will absolutely do
> "bad things".
> 
> Perhaps we should update the comment in dw_pcie_ep_set_bar():
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7e7844ff0f7e..451ba8add157 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -518,6 +518,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>                 /*
>                  * We can only dynamically change a BAR if the new BAR size and
>                  * BAR flags do not differ from the existing configuration.
> +                *
> +                * Note: this safety check only works when the caller uses a new
> +                * struct pci_epf_bar in the second set_bar() call. If the same
> +                * struct pci_epf_bar was supplied (i.e. being updated in place)
> +                * then it is impossible to detect invalid changes to the BAR.
>                  */
>                 if (ep_func->epf_bar[bar]->barno != bar ||
>                     ep_func->epf_bar[bar]->size != size ||
> 
> 
> To make it clear that this safety check is not always possible.
> 
> 
> > > I'm sorry for making you waste time. I did miss that even though pci-epf-vntb
> > > does not do in place updates of doorbell BAR, it does so for the other BARs.
> > 
> > No worries at all, and thanks for digging through the history with me.
> > At this point, I think there are still two reasonable options (to
> > summarize):
> > 
> > X). Treat the existing in-tree callers (including in-place update) as valid
> >     usage (i.e. apply [4]).
> > 
> >     [4] https://lore.kernel.org/linux-pci/q5e7ydmf4ra6x2mbxwifovgr6p6x5dfnz3hz5psq5ypyabtsvx@oq5ovi4o26yf/
> > 
> >     In this case, the downside noted in [4] remains: if a BAR reprogramming
> >     attempt fails (especially for the long-standing epf-vntb's BAR Match ->
> >     BAR Match transition case), the previously programmed inbound mapping
> >     will already have been torn down. This behavior change is inherent in
> >     making the teardown unconditional. I think this is acceptable because
> >     if the caller is passing incompatible/invalid parameters, things are
> >     already going off the rails anyway, and the call site that receives the
> >     error should never actively use the BAR for any real transactions.
> > 
> >     Separately, if we treat in-place updates as supported, some of the
> >     existing compatibility checks (e.g. barno/size/flags) become inherently
> >     best-effort, because the previous state may no longer be observable by
> >     the driver. Addressing that would require additional follow-up work
> >     (e.g. with doing a kmemdup and holding the snapshot), but this is a
> >     pre-existing issue, so there is no need to rush fixing this.
> > 
> > Y). Define a stricter API usage contract, document it, and then adjust all
> >     the caller sides later (i.e. apply this v2 series).
> > 
> >     The downside here is that struct pci_epf embeds the struct pci_epf_bar
> >     array, so tightening the contract and fixing existing users would
> >     likely be awkward.
> > 
> > Personally, I'm inclined towards (X) at the moment, mainly because there
> > doesn't seem to be a firm, shared understanding of the API contract today.
> > Later, we can do follow-up work for the existing behaviour, which is
> > already present on mainline.
> > 
> > If you still agree with (X), I'll send v2 with splitting [4] into two-patch
> > series, with an explanation above the unconditional
> > dw_pcie_ep_clear_ib_maps().
> 
> I did not change my mind a second time :)
> 
> So I still think X is the way to go.

I have just sent v2:
https://lore.kernel.org/linux-pci/20260202145407.503348-1-den@valinux.co.jp/
Thanks for the review.

Kind regards,
Koichiro

> 
> 
> Kind regards,
> Niklas