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@xxxxxxxxxxxxx/
Thanks for the review.

Kind regards,
Koichiro

> 
> 
> Kind regards,
> Niklas