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

[Niklas Cassel <cassel@kernel.org>]

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