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

[Koichiro Den]

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@xxxxxxx>
> 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@xxxxxxx>
> 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@xxxxxxx>
> 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