Message-ID: <4erlj426nvmilwfdq5e63ojiqecomcpj35nvmiyw2p5mvifwlt@yspmfxrzmxei>
Date: Mon, 2 Feb 2026 00:45:52 +0900
From: Koichiro Den <den@...inux.co.jp>
To: Niklas Cassel <cassel@...nel.org>
Cc: mani@...nel.org, kwilczynski@...nel.org, kishon@...nel.org, 
	bhelgaas@...gle.com, corbet@....net, jingoohan1@...il.com, lpieralisi@...nel.org, 
	robh@...nel.org, Frank.Li@....com, linux-pci@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] PCI: endpoint: Document pci_epc_set_bar() caller
 ownership and lifetime rules

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@...inux.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

Powered by blists - more mailing lists