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

On Mon, Feb 02, 2026 at 02:59:35PM +0900, Koichiro Den wrote:

quoted

quoted

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/ (local)
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.

quoted

quoted

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/ (local)

    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.