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.0On 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
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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.