On Fri, Jan 23, 2026 at 11:28:30PM +0900, Koichiro Den wrote:
> On Fri, Jan 23, 2026 at 09:51:19AM +0100, Niklas Cassel wrote:
> > On Fri, Jan 23, 2026 at 10:16:21AM +0900, Koichiro Den wrote:
> > > >
> > > > There might be other EPC drivers that don't disable all BARs in their
> > > > .init(), so I would say that simply checking if the BAR has an address
> > > > is not sufficient to guarantee that an EPF driver has called set_bar().
> > > >
> > >
> > > Even if an EPC driver does not reset the BAR in their .init() and some
> > > default translation is left exposed, wouldn't it be safe as long as
> > > dw_pcie_ep_ib_atu_addr() succeeds in programming inbound mappings for the
> > > entire BAR?
> >
> > For e.g. on RK3588, the default HW configuration of the DWC controller has
> > all 5 BARs as enabled, with a size of 1 GB.
> >
> > There is no inbound address translation for these BARs by default.
> >
> > So for it to be safe, the size of the set_bar() call would have to
> > match the current size of the BAR, but how should the EPF driver know
> > that when it has not called set_bar() yet?
> >
> > dw_pcie_ep_read_bar_assigned() does not return the current size of the
> > BAR. So you can't verify that the set_bar() call has the same size as
> > the BARs "default size".
>
> I wasn't considering either of the following cases as unsafe:
> - succeeding by chance in programming via a one-shot set_bar() with submaps
> - such a set_bar() failing (due to incorrect size recognition)
>
> while as I mentioned in my previous reply, the first case effectively
> becomes a loophole that contradicts the docs and git commit messages.
>
> However, since v8, the second case clears any existing mappings, which
> could indeed lead to an unsafe situtation.
>
> >
> >
> > >
> > > That said, such usage apparently contradicts the documented usage (1st
> > > set_bar with no submap, then with submap) described in the docs and commit
> > > messages in this series, and allowing it would make things unnecessarily
> > > complicated. So I agree that adding such a safeguard is the right
> > > approach.
> > >
> > > >
> > > > I think the safest option is my second suggestion because then we know
> > > > that we will only call
> > > > dw_pcie_ep_ib_atu_addr()
> > > >
> > > > When:
> > > >
> > > > 1) If ep->epf_bar[bar] is set:
> > > > https://github.com/torvalds/linux/blob/v6.19-rc6/drivers/pci/controller/dwc/pcie-designware-ep.c#L363
> > > >
> > > >
> > > > 2) All the other requirements to dynamically update a BAR is also met:
> > > >
> > > > https://github.com/torvalds/linux/blob/v6.19-rc6/drivers/pci/controller/dwc/pcie-designware-ep.c#L368-L370
> > > >
> > >
> > > That makes sense, and it ensures that the behavior always accords with the
> > > docs and commit messages in this series.
> >
> > I think it makes most sense to put the "use_addr_translation = true"
> >
> > after the check:
> >
> > /*
> > * We can only dynamically change a BAR if the new BAR size and
> > * BAR flags do not differ from the existing configuration.
> > */
> > if (ep->epf_bar[bar]->barno != bar ||
> > ep->epf_bar[bar]->size != size ||
> > ep->epf_bar[bar]->flags != flags)
> > return -EINVAL;
> >
> >
> > So we know that dw_pcie_ep_ib_atu_addr() is only called when the size is the
> > same.
>
> I'll send v10 with the fix, possibly adding a BAR_SUBRANGE_TEST to pci
> endpoint test as well.
After thinking again, I believe just the following is the most robust and
safest approach, as it makes subrange mapping strictly update-only and
avoids any silent success on invalid first-time calls.
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -508,20 +508,29 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8
func_no, u8 vfunc_no,
* mappings before re-programming.
*/
if (ep->epf_bar[bar]->num_submap || epf_bar->num_submap)
dw_pcie_ep_clear_ib_maps(ep, bar);
/*
* When dynamically changing a BAR, skip writing the BAR reg, as
* that would clear the BAR's PCI address assigned by the host.
*/
goto config_atu;
+ } else {
+ /*
+ * Subrange mapping is an update-only operation.
+ * The BAR must have been configured once without submaps so
that
+ * subsequent set_bar() calls can update inbound mappings
without
+ * touching the BAR register (and clobbering the host-assigned
address).
+ */
+ if (epf_bar->num_submap)
+ return -EINVAL;
}
bar_type = dw_pcie_ep_get_bar_type(ep, bar);
switch (bar_type) {
case BAR_FIXED:
/*
* There is no need to write a BAR mask for a fixed BAR (except
* to write 1 to the LSB of the BAR mask register, to enable the
* BAR). Write the BAR mask regardless. (The fixed bits in the
* BAR mask register will be read-only anyway.)
This is close to your first suggestion at:
https://lore.kernel.org/linux-pci/aXHsd7-WWAGyhy_w@ryzen/
but it avoids even performing BAR sizing when set_bar() is called in an invalid
manner.
With this, we still guarantee dw_pcie_ep_ib_atu_addr() is only reached when:
1) ep->epf_bar[bar] is set
2) All the other requirements to dynamically update a BAR is also met
The resulting behavior matrix becomes:
| num_submap > 0 | num_submap == 0
|
-------------------------+--------------------------+--------------------------+
ep->epf_bar[bar] == NULL | returns -EINVAL | always try BAR Match
|
ep->epf_bar[bar] != NULL | always try Address Match | always try BAR Match
|
By contrast, with the latest idea that relies on the local
"use_addr_translation" variable, the case marked as [1] below possibly
leads to an unexpected success in BAR Match Mode, .submap/.num_submap are
silently ignored, and the caller has no way to notice the mistake.
| num_submap > 0 | num_submap == 0
|
-------------------------+--------------------------+--------------------------+
ep->epf_bar[bar] == NULL | always try BAR Match [1] | always try BAR Match
|
ep->epf_bar[bar] != NULL | always try Address Match | always try BAR Match
|
Kind regards,
Koichiro
>
> Kind regards,
> Koichiro
>
> >
> >
> > Kind regards,
> > Niklas
