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

Koichiro Den
2026-02-02T15:04:14Z

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