* [PATCH v4 0/3] PCI: dwc: ep: Enhance multi-function endpoint support
@ 2026-01-29 9:17 Aksh Garg
2026-01-29 9:17 ` [PATCH v4 1/3] PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations Aksh Garg
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Aksh Garg @ 2026-01-29 9:17 UTC (permalink / raw)
To: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh,
bhelgaas, cassel, Zhiqiang.Hou, gustavo.pimentel
Cc: linux-kernel, s-vadapalli, danishanwar, Aksh Garg
This series addresses multi-function endpoint configuration issues in
the DWC PCIe controller driver. The changes enable proper operations
for physical functions and enhance the multi-function endpoint support.
Changes from v3 to v4:
- Rebased the series
Changes from v2 to v3:
- Fixed minor nits in the patch
Changes from v1 to v2:
- Fixed the minor nits in the patches
- Added a new patch in the series to address PTM capability access
v3: https://lore.kernel.org/all/20260127085010.446116-1-a-garg7@ti.com/
v2: https://lore.kernel.org/all/20260122082538.309122-1-a-garg7@ti.com/
v1: https://lore.kernel.org/all/20260121054214.274429-1-a-garg7@ti.com/
Aksh Garg (3):
PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations
PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support
PCI: dwc: ep: Add comment explaining controller-level PTM access
.../pci/controller/dwc/pcie-designware-ep.c | 132 ++++++++++++------
drivers/pci/controller/dwc/pcie-designware.h | 12 +-
2 files changed, 92 insertions(+), 52 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v4 1/3] PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations 2026-01-29 9:17 [PATCH v4 0/3] PCI: dwc: ep: Enhance multi-function endpoint support Aksh Garg @ 2026-01-29 9:17 ` Aksh Garg 2026-01-29 9:17 ` [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support Aksh Garg 2026-01-29 9:17 ` [PATCH v4 3/3] PCI: dwc: ep: Add comment explaining controller-level PTM access Aksh Garg 2 siblings, 0 replies; 15+ messages in thread From: Aksh Garg @ 2026-01-29 9:17 UTC (permalink / raw) To: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, cassel, Zhiqiang.Hou, gustavo.pimentel Cc: linux-kernel, s-vadapalli, danishanwar, Aksh Garg The resizable BAR support added by the commit 3a3d4cabe681 ("PCI: dwc: ep: Allow EPF drivers to configure the size of Resizable BARs") incorrectly configures the resizable BARs only for the first Physical Function (PF0) in EP mode. The resizable BAR configuration functions use generic dw_pcie_*_dbi operations instead of physical function specific dw_pcie_ep_*_dbi operations. This causes resizable BAR configuration to always target PF0 regardless of the requested function number. Additionally, dw_pcie_ep_init_non_sticky_registers() only initializes resizable BAR registers for PF0, leaving other PFs unconfigured during the execution of this function. Fix this by using physical function specific configuration space access operations throughout the resizable BAR code path and initializing registers for all the physical functions that support resizable BARs. Fixes: 3a3d4cabe681 ("PCI: dwc: ep: Allow EPF drivers to configure the size of Resizable BARs") Signed-off-by: Aksh Garg <a-garg7@ti.com> Reviewed-by: Niklas Cassel <cassel@kernel.org> --- Changes from v3 to v4: - Rebased the patch, no functional change Changes from v2 to v3: - Changed function name from __dw_pcie_ep_init_non_sticky_registers() to dw_pcie_ep_init_rebar_registers() Changes from v1 to v2: - Fixed the suggested nit v3: https://lore.kernel.org/all/20260127085010.446116-2-a-garg7@ti.com/ v2: https://lore.kernel.org/all/20260122082538.309122-2-a-garg7@ti.com/ v1: https://lore.kernel.org/all/20260121054214.274429-2-a-garg7@ti.com/ .../pci/controller/dwc/pcie-designware-ep.c | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index de09cd786edc..1b3dd07e6004 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -75,6 +75,13 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie_ep *ep, u8 func_no, u8 cap) cap, NULL, ep, func_no); } +static u16 dw_pcie_ep_find_ext_capability(struct dw_pcie_ep *ep, + u8 func_no, u8 cap) +{ + return PCI_FIND_NEXT_EXT_CAP(dw_pcie_ep_read_cfg, 0, + cap, NULL, ep, func_no); +} + static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no, struct pci_epf_header *hdr) { @@ -350,22 +357,22 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, ep->epf_bar[bar] = NULL; } -static unsigned int dw_pcie_ep_get_rebar_offset(struct dw_pcie *pci, +static unsigned int dw_pcie_ep_get_rebar_offset(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) { u32 reg, bar_index; unsigned int offset, nbars; int i; - offset = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); + offset = dw_pcie_ep_find_ext_capability(ep, func_no, PCI_EXT_CAP_ID_REBAR); if (!offset) return offset; - reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL); + reg = dw_pcie_ep_readl_dbi(ep, func_no, offset + PCI_REBAR_CTRL); nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, reg); for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL) { - reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL); + reg = dw_pcie_ep_readl_dbi(ep, func_no, offset + PCI_REBAR_CTRL); bar_index = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, reg); if (bar_index == bar) return offset; @@ -386,7 +393,7 @@ static int dw_pcie_ep_set_bar_resizable(struct dw_pcie_ep *ep, u8 func_no, u32 rebar_cap, rebar_ctrl; int ret; - rebar_offset = dw_pcie_ep_get_rebar_offset(pci, bar); + rebar_offset = dw_pcie_ep_get_rebar_offset(ep, func_no, bar); if (!rebar_offset) return -EINVAL; @@ -416,16 +423,16 @@ static int dw_pcie_ep_set_bar_resizable(struct dw_pcie_ep *ep, u8 func_no, * 1 MB to 128 TB. Bits 31:16 in PCI_REBAR_CTRL define "supported sizes" * bits for sizes 256 TB to 8 EB. Disallow sizes 256 TB to 8 EB. */ - rebar_ctrl = dw_pcie_readl_dbi(pci, rebar_offset + PCI_REBAR_CTRL); + rebar_ctrl = dw_pcie_ep_readl_dbi(ep, func_no, rebar_offset + PCI_REBAR_CTRL); rebar_ctrl &= ~GENMASK(31, 16); - dw_pcie_writel_dbi(pci, rebar_offset + PCI_REBAR_CTRL, rebar_ctrl); + dw_pcie_ep_writel_dbi(ep, func_no, rebar_offset + PCI_REBAR_CTRL, rebar_ctrl); /* * The "selected size" (bits 13:8) in PCI_REBAR_CTRL are automatically * updated when writing PCI_REBAR_CAP, see "Figure 3-26 Resizable BAR * Example for 32-bit Memory BAR0" in DWC EP databook 5.96a. */ - dw_pcie_writel_dbi(pci, rebar_offset + PCI_REBAR_CAP, rebar_cap); + dw_pcie_ep_writel_dbi(ep, func_no, rebar_offset + PCI_REBAR_CAP, rebar_cap); dw_pcie_dbi_ro_wr_dis(pci); @@ -1022,20 +1029,17 @@ void dw_pcie_ep_deinit(struct dw_pcie_ep *ep) } EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit); -static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci) +static void dw_pcie_ep_init_rebar_registers(struct dw_pcie_ep *ep, u8 func_no) { - struct dw_pcie_ep *ep = &pci->ep; unsigned int offset; unsigned int nbars; enum pci_barno bar; u32 reg, i, val; - offset = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); - - dw_pcie_dbi_ro_wr_en(pci); + offset = dw_pcie_ep_find_ext_capability(ep, func_no, PCI_EXT_CAP_ID_REBAR); if (offset) { - reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL); + reg = dw_pcie_ep_readl_dbi(ep, func_no, offset + PCI_REBAR_CTRL); nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, reg); /* @@ -1056,16 +1060,28 @@ static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci) * the controller when RESBAR_CAP_REG is written, which * is why RESBAR_CAP_REG is written here. */ - val = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL); + val = dw_pcie_ep_readl_dbi(ep, func_no, offset + PCI_REBAR_CTRL); bar = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, val); if (ep->epf_bar[bar]) pci_epc_bar_size_to_rebar_cap(ep->epf_bar[bar]->size, &val); else val = BIT(4); - dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, val); + dw_pcie_ep_writel_dbi(ep, func_no, offset + PCI_REBAR_CAP, val); } } +} + +static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci) +{ + struct dw_pcie_ep *ep = &pci->ep; + u8 funcs = ep->epc->max_functions; + u8 func_no; + + dw_pcie_dbi_ro_wr_en(pci); + + for (func_no = 0; func_no < funcs; func_no++) + dw_pcie_ep_init_rebar_registers(ep, func_no); dw_pcie_setup(pci); dw_pcie_dbi_ro_wr_dis(pci); -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-29 9:17 [PATCH v4 0/3] PCI: dwc: ep: Enhance multi-function endpoint support Aksh Garg 2026-01-29 9:17 ` [PATCH v4 1/3] PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations Aksh Garg @ 2026-01-29 9:17 ` Aksh Garg 2026-01-29 14:14 ` Niklas Cassel 2026-01-29 9:17 ` [PATCH v4 3/3] PCI: dwc: ep: Add comment explaining controller-level PTM access Aksh Garg 2 siblings, 1 reply; 15+ messages in thread From: Aksh Garg @ 2026-01-29 9:17 UTC (permalink / raw) To: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, cassel, Zhiqiang.Hou, gustavo.pimentel Cc: linux-kernel, s-vadapalli, danishanwar, Aksh Garg The commit 24ede430fa49 ("PCI: designware-ep: Add multiple PFs support for DWC") added support for multiple PFs in the DWC driver, but the implementation was incomplete. It did not properly support MSI/MSI-X, as well as BAR and inbound ATU mapping for multiple PFs. The MSI/MSI-X issue was later fixed by commit 47a062609a30 ("PCI: designware-ep: Modify MSI and MSIX CAP way of finding") by introducing a per-PF struct dw_pcie_ep_func. However, even with both commits, the multiple PF support in the driver remains broken because BAR configuration and ATU mappings are managed globally in struct dw_pcie_ep, meaning all PFs share the same BAR-to-ATU mapping table. This causes one PF's EPF to overwrite the address translation of another PF's EPF in the internal ATU region, creating conflicts when multiple physical functions attempt to configure their BARs independently. The commit cfbc98dbf44d ("PCI: dwc: ep: Support BAR subrange inbound mapping via Address Match Mode iATU") later introduced Address Match Mode support, which suffers from the same multi-PF conflict issue. Fix this by moving the required members from struct dw_pcie_ep to struct dw_pcie_ep_func, similar to what commit 47a062609a30 ("PCI: designware-ep: Modify MSI and MSIX CAP way of finding") did for MSI/MSI-X capability support, to allow proper multi-function endpoint operation, where each PF can configure its BARs and corresponding internal ATU region without interfering with other PFs. Fixes: 24ede430fa49 ("PCI: designware-ep: Add multiple PFs support for DWC") Fixes: cfbc98dbf44d ("PCI: dwc: ep: Support BAR subrange inbound mapping via Address Match Mode iATU") Signed-off-by: Aksh Garg <a-garg7@ti.com> Reviewed-by: Niklas Cassel <cassel@kernel.org> --- Changes from v3 to v4: - Fix the similar conflict created by the commit cfbc98dbf44d Changes from v2 to v3: - None Changes from v1 to v2: - Fixed the suggested nits - Rephrased the commit message with a proper Fixes tag v3: https://lore.kernel.org/all/20260127085010.446116-3-a-garg7@ti.com/ v2: https://lore.kernel.org/all/20260122082538.309122-3-a-garg7@ti.com/ v1: https://lore.kernel.org/all/20260121054214.274429-3-a-garg7@ti.com/ .../pci/controller/dwc/pcie-designware-ep.c | 76 +++++++++++-------- drivers/pci/controller/dwc/pcie-designware.h | 12 +-- 2 files changed, 51 insertions(+), 37 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 1b3dd07e6004..b9a234b47aab 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -115,11 +115,15 @@ static int dw_pcie_ep_ib_atu_bar(struct dw_pcie_ep *ep, u8 func_no, int type, int ret; u32 free_win; struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); - if (!ep->bar_to_atu[bar]) + if (!ep_func) + return -EINVAL; + + if (!ep_func->bar_to_atu[bar]) free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows); else - free_win = ep->bar_to_atu[bar] - 1; + free_win = ep_func->bar_to_atu[bar] - 1; if (free_win >= pci->num_ib_windows) { dev_err(pci->dev, "No free inbound window\n"); @@ -137,14 +141,15 @@ static int dw_pcie_ep_ib_atu_bar(struct dw_pcie_ep *ep, u8 func_no, int type, * Always increment free_win before assignment, since value 0 is used to identify * unallocated mapping. */ - ep->bar_to_atu[bar] = free_win + 1; + ep_func->bar_to_atu[bar] = free_win + 1; set_bit(free_win, ep->ib_window_map); return 0; } -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) { + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); struct dw_pcie *pci = to_dw_pcie_from_ep(ep); struct device *dev = pci->dev; unsigned int i, num; @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) u32 *indexes; /* Tear down the BAR Match Mode mapping, if any. */ - if (ep->bar_to_atu[bar]) { - atu_index = ep->bar_to_atu[bar] - 1; + if (ep_func->bar_to_atu[bar]) { + atu_index = ep_func->bar_to_atu[bar] - 1; dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index); clear_bit(atu_index, ep->ib_window_map); - ep->bar_to_atu[bar] = 0; + ep_func->bar_to_atu[bar] = 0; } /* Tear down all Address Match Mode mappings, if any. */ - indexes = ep->ib_atu_indexes[bar]; - num = ep->num_ib_atu_indexes[bar]; - ep->ib_atu_indexes[bar] = NULL; - ep->num_ib_atu_indexes[bar] = 0; + indexes = ep_func->ib_atu_indexes[bar]; + num = ep_func->num_ib_atu_indexes[bar]; + ep_func->ib_atu_indexes[bar] = NULL; + ep_func->num_ib_atu_indexes[bar] = 0; if (!indexes) return; for (i = 0; i < num; i++) { @@ -248,6 +253,7 @@ static int dw_pcie_ep_validate_submap(struct dw_pcie_ep *ep, static int dw_pcie_ep_ib_atu_addr(struct dw_pcie_ep *ep, u8 func_no, int type, const struct pci_epf_bar *epf_bar) { + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); const struct pci_epf_bar_submap *submap = epf_bar->submap; struct dw_pcie *pci = to_dw_pcie_from_ep(ep); enum pci_barno bar = epf_bar->barno; @@ -258,7 +264,7 @@ static int dw_pcie_ep_ib_atu_addr(struct dw_pcie_ep *ep, u8 func_no, int type, unsigned int i; u32 *indexes; - if (!epf_bar->num_submap || !submap || !epf_bar->size) + if (!ep_func || !epf_bar->num_submap || !submap || !epf_bar->size) return -EINVAL; ret = dw_pcie_ep_validate_submap(ep, submap, epf_bar->num_submap, @@ -279,8 +285,8 @@ static int dw_pcie_ep_ib_atu_addr(struct dw_pcie_ep *ep, u8 func_no, int type, if (!indexes) return -ENOMEM; - ep->ib_atu_indexes[bar] = indexes; - ep->num_ib_atu_indexes[bar] = 0; + ep_func->ib_atu_indexes[bar] = indexes; + ep_func->num_ib_atu_indexes[bar] = 0; for (i = 0; i < epf_bar->num_submap; i++) { size = submap[i].size; @@ -308,11 +314,11 @@ static int dw_pcie_ep_ib_atu_addr(struct dw_pcie_ep *ep, u8 func_no, int type, set_bit(free_win, ep->ib_window_map); indexes[i] = free_win; - ep->num_ib_atu_indexes[bar] = i + 1; + ep_func->num_ib_atu_indexes[bar] = i + 1; } return 0; err: - dw_pcie_ep_clear_ib_maps(ep, bar); + dw_pcie_ep_clear_ib_maps(ep, func_no, bar); return ret; } @@ -346,15 +352,16 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, struct dw_pcie_ep *ep = epc_get_drvdata(epc); struct dw_pcie *pci = to_dw_pcie_from_ep(ep); enum pci_barno bar = epf_bar->barno; + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); - if (!ep->epf_bar[bar]) + if (!ep_func || !ep_func->epf_bar[bar]) return; __dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags); - dw_pcie_ep_clear_ib_maps(ep, bar); + dw_pcie_ep_clear_ib_maps(ep, func_no, bar); - ep->epf_bar[bar] = NULL; + ep_func->epf_bar[bar] = NULL; } static unsigned int dw_pcie_ep_get_rebar_offset(struct dw_pcie_ep *ep, u8 func_no, @@ -481,12 +488,16 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, { struct dw_pcie_ep *ep = epc_get_drvdata(epc); struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); enum pci_barno bar = epf_bar->barno; size_t size = epf_bar->size; enum pci_epc_bar_type bar_type; int flags = epf_bar->flags; int ret, type; + if (!ep_func) + return -EINVAL; + /* * DWC does not allow BAR pairs to overlap, e.g. you cannot combine BARs * 1 and 2 to form a 64-bit BAR. @@ -500,22 +511,22 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, * calling clear_bar() would clear the BAR's PCI address assigned by the * host). */ - if (ep->epf_bar[bar]) { + if (ep_func->epf_bar[bar]) { /* * 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) + if (ep_func->epf_bar[bar]->barno != bar || + ep_func->epf_bar[bar]->size != size || + ep_func->epf_bar[bar]->flags != flags) return -EINVAL; /* * When dynamically changing a BAR, tear down any existing * mappings before re-programming. */ - if (ep->epf_bar[bar]->num_submap || epf_bar->num_submap) - dw_pcie_ep_clear_ib_maps(ep, bar); + if (ep_func->epf_bar[bar]->num_submap || epf_bar->num_submap) + dw_pcie_ep_clear_ib_maps(ep, func_no, bar); /* * When dynamically changing a BAR, skip writing the BAR reg, as @@ -573,7 +584,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, if (ret) return ret; - ep->epf_bar[bar] = epf_bar; + ep_func->epf_bar[bar] = epf_bar; return 0; } @@ -968,7 +979,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset); tbl_offset &= PCI_MSIX_TABLE_OFFSET; - msix_tbl = ep->epf_bar[bir]->addr + tbl_offset; + msix_tbl = ep_func->epf_bar[bir]->addr + tbl_offset; msg_addr = msix_tbl[(interrupt_num - 1)].msg_addr; msg_data = msix_tbl[(interrupt_num - 1)].msg_data; vec_ctrl = msix_tbl[(interrupt_num - 1)].vector_ctrl; @@ -1031,11 +1042,14 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit); static void dw_pcie_ep_init_rebar_registers(struct dw_pcie_ep *ep, u8 func_no) { - unsigned int offset; - unsigned int nbars; + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); + unsigned int offset, nbars; enum pci_barno bar; u32 reg, i, val; + if (!ep_func) + return; + offset = dw_pcie_ep_find_ext_capability(ep, func_no, PCI_EXT_CAP_ID_REBAR); if (offset) { @@ -1062,8 +1076,8 @@ static void dw_pcie_ep_init_rebar_registers(struct dw_pcie_ep *ep, u8 func_no) */ val = dw_pcie_ep_readl_dbi(ep, func_no, offset + PCI_REBAR_CTRL); bar = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, val); - if (ep->epf_bar[bar]) - pci_epc_bar_size_to_rebar_cap(ep->epf_bar[bar]->size, &val); + if (ep_func->epf_bar[bar]) + pci_epc_bar_size_to_rebar_cap(ep_func->epf_bar[bar]->size, &val); else val = BIT(4); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 8f170122ad78..43d7606bc987 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -471,6 +471,12 @@ struct dw_pcie_ep_func { u8 func_no; u8 msi_cap; /* MSI capability offset */ u8 msix_cap; /* MSI-X capability offset */ + u8 bar_to_atu[PCI_STD_NUM_BARS]; + struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; + + /* Only for Address Match Mode inbound iATU */ + u32 *ib_atu_indexes[PCI_STD_NUM_BARS]; + unsigned int num_ib_atu_indexes[PCI_STD_NUM_BARS]; }; struct dw_pcie_ep { @@ -480,17 +486,11 @@ struct dw_pcie_ep { phys_addr_t phys_base; size_t addr_size; size_t page_size; - u8 bar_to_atu[PCI_STD_NUM_BARS]; phys_addr_t *outbound_addr; unsigned long *ib_window_map; unsigned long *ob_window_map; void __iomem *msi_mem; phys_addr_t msi_mem_phys; - struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; - - /* Only for Address Match Mode inbound iATU */ - u32 *ib_atu_indexes[PCI_STD_NUM_BARS]; - unsigned int num_ib_atu_indexes[PCI_STD_NUM_BARS]; /* MSI outbound iATU state */ bool msi_iatu_mapped; -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-29 9:17 ` [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support Aksh Garg @ 2026-01-29 14:14 ` Niklas Cassel 2026-01-30 2:21 ` Koichiro Den 2026-01-30 4:12 ` Aksh Garg 0 siblings, 2 replies; 15+ messages in thread From: Niklas Cassel @ 2026-01-29 14:14 UTC (permalink / raw) To: Aksh Garg, Koichiro Den Cc: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) > { > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > struct device *dev = pci->dev; > unsigned int i, num; > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > u32 *indexes; Hello Aksh, Considering that all other functions that you have modified, you have added a: if (!ep_func) return; I think you should do the same to this function. > > /* Tear down the BAR Match Mode mapping, if any. */ > - if (ep->bar_to_atu[bar]) { > - atu_index = ep->bar_to_atu[bar] - 1; > + if (ep_func->bar_to_atu[bar]) { > + atu_index = ep_func->bar_to_atu[bar] - 1; > dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index); > clear_bit(atu_index, ep->ib_window_map); > - ep->bar_to_atu[bar] = 0; > + ep_func->bar_to_atu[bar] = 0; Not related to your patch, Koichiro (he is on To:), don't you think that it would be clearer if we had a: return; here... I mean, a BAR can either have a BAR match mode mapping or a subrange mapping, but not both... So continuing executing code beyond this point seems pointless, possibly even confusing. > } > > /* Tear down all Address Match Mode mappings, if any. */ > - indexes = ep->ib_atu_indexes[bar]; > - num = ep->num_ib_atu_indexes[bar]; > - ep->ib_atu_indexes[bar] = NULL; > - ep->num_ib_atu_indexes[bar] = 0; > + indexes = ep_func->ib_atu_indexes[bar]; > + num = ep_func->num_ib_atu_indexes[bar]; > + ep_func->ib_atu_indexes[bar] = NULL; > + ep_func->num_ib_atu_indexes[bar] = 0; > if (!indexes) > return; Sure, I see that this code will do a return here... So there will be no harm done, but, having a simple return above would make it extra clear to the reader that is is always one or the other... you cannot have both. If you agree, perhaps you could send a one liner patch? > for (i = 0; i < num; i++) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-29 14:14 ` Niklas Cassel @ 2026-01-30 2:21 ` Koichiro Den 2026-01-30 9:57 ` Niklas Cassel 2026-01-30 4:12 ` Aksh Garg 1 sibling, 1 reply; 15+ messages in thread From: Koichiro Den @ 2026-01-30 2:21 UTC (permalink / raw) To: Niklas Cassel Cc: Aksh Garg, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Thu, Jan 29, 2026 at 03:14:51PM +0100, Niklas Cassel wrote: > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: > > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) > > { > > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > struct device *dev = pci->dev; > > unsigned int i, num; > > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > u32 *indexes; > > Hello Aksh, > > Considering that all other functions that you have modified, you have added a: > > if (!ep_func) > return; > > > I think you should do the same to this function. > > > > > > /* Tear down the BAR Match Mode mapping, if any. */ > > - if (ep->bar_to_atu[bar]) { > > - atu_index = ep->bar_to_atu[bar] - 1; > > + if (ep_func->bar_to_atu[bar]) { > > + atu_index = ep_func->bar_to_atu[bar] - 1; > > dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index); > > clear_bit(atu_index, ep->ib_window_map); > > - ep->bar_to_atu[bar] = 0; > > + ep_func->bar_to_atu[bar] = 0; > > Not related to your patch, > > Koichiro (he is on To:), > > don't you think that it would be clearer if we had a: > return; > > here... > > I mean, a BAR can either have a BAR match mode mapping or a subrange mapping, > but not both... So continuing executing code beyond this point seems pointless, > possibly even confusing. Thanks for pinging. I agree it would be clearer. Since it's orthogonal to this series, would it be better if I sent a one-line patch? By the way, Aksh, thank you for addressing the newly added address-match mode case. Koichiro > > > > } > > > > /* Tear down all Address Match Mode mappings, if any. */ > > - indexes = ep->ib_atu_indexes[bar]; > > - num = ep->num_ib_atu_indexes[bar]; > > - ep->ib_atu_indexes[bar] = NULL; > > - ep->num_ib_atu_indexes[bar] = 0; > > + indexes = ep_func->ib_atu_indexes[bar]; > > + num = ep_func->num_ib_atu_indexes[bar]; > > + ep_func->ib_atu_indexes[bar] = NULL; > > + ep_func->num_ib_atu_indexes[bar] = 0; > > if (!indexes) > > return; > > Sure, I see that this code will do a return here... > > So there will be no harm done, but, having a simple return above > would make it extra clear to the reader that is is always one or > the other... you cannot have both. > > If you agree, perhaps you could send a one liner patch? > > > > for (i = 0; i < num; i++) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 2:21 ` Koichiro Den @ 2026-01-30 9:57 ` Niklas Cassel 2026-01-30 17:16 ` Koichiro Den 0 siblings, 1 reply; 15+ messages in thread From: Niklas Cassel @ 2026-01-30 9:57 UTC (permalink / raw) To: Koichiro Den Cc: Aksh Garg, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Fri, Jan 30, 2026 at 11:21:32AM +0900, Koichiro Den wrote: > > > > Koichiro (he is on To:), > > > > don't you think that it would be clearer if we had a: > > return; > > > > here... > > > > I mean, a BAR can either have a BAR match mode mapping or a subrange mapping, > > but not both... So continuing executing code beyond this point seems pointless, > > possibly even confusing. > > Thanks for pinging. I agree it would be clearer. Since it's orthogonal to > this series, would it be better if I sent a one-line patch? Thank you Koichiro, I think a one liner patch for this would be nice. You can also write in the changelog (after the ----) that Mani can squash your one liner patch into the existing submap commit if he wants. Kind regards, Niklas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 9:57 ` Niklas Cassel @ 2026-01-30 17:16 ` Koichiro Den 2026-01-30 22:51 ` Niklas Cassel 0 siblings, 1 reply; 15+ messages in thread From: Koichiro Den @ 2026-01-30 17:16 UTC (permalink / raw) To: Niklas Cassel Cc: Aksh Garg, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Fri, Jan 30, 2026 at 10:57:18AM +0100, Niklas Cassel wrote: > On Fri, Jan 30, 2026 at 11:21:32AM +0900, Koichiro Den wrote: > > > > > > Koichiro (he is on To:), > > > > > > don't you think that it would be clearer if we had a: > > > return; > > > > > > here... > > > > > > I mean, a BAR can either have a BAR match mode mapping or a subrange mapping, > > > but not both... So continuing executing code beyond this point seems pointless, > > > possibly even confusing. > > > > Thanks for pinging. I agree it would be clearer. Since it's orthogonal to > > this series, would it be better if I sent a one-line patch? > > Thank you Koichiro, I think a one liner patch for this would be nice. > > You can also write in the changelog (after the ----) that Mani can squash > your one liner patch into the existing submap commit if he wants. While looking at this, I spotted a subtle corner case that the missing 'return' has been masking. Consider the following re-programming sequence for a BAR: 1) The initial set_bar() programs the BAR in BAR Match Mode 2) The second set_bar() switches the BAR to Address Match Mode - Ad part of this, set_bar() calls dw_pcie_ep_clear_ib_maps() before re-programming. 3) The third set_bar() switches back to BAR Match Mode. In step (3), the current behavior depends on how the caller handles the struct pci_epf_bar passed to set_bar() a) If the caller passes a freshly prepared epf_bar with .num_submap = 0, dw_pcie_ep_clear_ib_maps() is called as expected. b) If the caller updates the same epf_bar in place (i.e. reused the object that passed in step (2)) and simply updates .num_submap back to 0, dw_pcie_ep_clear_ib_maps() is NOT called because the condition in [1] evaluates to false. The recently merged (my) pci-epf-test code follows this patter, see [2]. The point is that ep_func->epf_bar[bar] only stores a pointer, and the actual object is owned by the API caller. Since struct pci_epf embeds the BAR descriptors, updating an epf_bar in place (pattern (b)) seems quite natural and, in my view, should be supported. Requiring callers to always pass a new epf_bar (pattern (a)) feels contrary to the current API/data-structure design. Given that, I think the cleares fix is: diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index dd48e60b513c..a20cff98b797 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -165,6 +165,9 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index); clear_bit(atu_index, ep->ib_window_map); ep_func->bar_to_atu[bar] = 0; + + WARN_ON(ep_func->ib_atu_indexes[bar]); + return; } /* Tear down all Address Match Mode mappings, if any. */ @@ -528,8 +531,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, * When dynamically changing a BAR, tear down any existing * mappings before re-programming. */ - if (ep_func->epf_bar[bar]->num_submap || epf_bar->num_submap) - dw_pcie_ep_clear_ib_maps(ep, func_no, bar); + dw_pcie_ep_clear_ib_maps(ep, func_no, bar); /* * When dynamically changing a BAR, skip writing the BAR reg, as ---8<--- One downside is that this introduces a behavioral change in a specific failure case that already existed prior to this series. Concretely, this only affects the traditional reprogramming path where a BAR configured in BAR Match Mode is updated again to BAR Match Mode. If such an update fails (for example, because the new epf_bar->phys_addr is not aligned to region_align), the behavior changes as follows: - Before this change: the previously programmed BAR Match Mode mapping remains in place. - After this change: the existing mapping is torn down first, so no mapping remains after the failure. I don't think this change is a major issue, since the caller will still observe the failure and should handle the error accordingly in either case. That said, I'd appreciate feedback if retaining the old BAR Match Mode mapping on such failure scenario is considered important for the existing update path. [1] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c?h=controller/dwc&id=0e2b9294512c140576c340b7ccd24230a593f30b#n531 [2] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/endpoint/functions/pci-epf-test.c?h=controller/dwc&id=0e2b9294512c140576c340b7ccd24230a593f30b#n950 Kind regards, Koichiro > > > Kind regards, > Niklas ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 17:16 ` Koichiro Den @ 2026-01-30 22:51 ` Niklas Cassel 2026-01-31 13:42 ` Koichiro Den 0 siblings, 1 reply; 15+ messages in thread From: Niklas Cassel @ 2026-01-30 22:51 UTC (permalink / raw) To: Koichiro Den Cc: Aksh Garg, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar Hello Koichiro, On Sat, Jan 31, 2026 at 02:16:37AM +0900, Koichiro Den wrote: > While looking at this, I spotted a subtle corner case that the missing > 'return' has been masking. > > Consider the following re-programming sequence for a BAR: > > 1) The initial set_bar() programs the BAR in BAR Match Mode > 2) The second set_bar() switches the BAR to Address Match Mode > - Ad part of this, set_bar() calls dw_pcie_ep_clear_ib_maps() before > re-programming. > 3) The third set_bar() switches back to BAR Match Mode. > > In step (3), the current behavior depends on how the caller handles the > struct pci_epf_bar passed to set_bar() > > a) If the caller passes a freshly prepared epf_bar with .num_submap = 0, > dw_pcie_ep_clear_ib_maps() is called as expected. > > b) If the caller updates the same epf_bar in place (i.e. reused the > object that passed in step (2)) and simply updates .num_submap back to > 0, dw_pcie_ep_clear_ib_maps() is NOT called because the condition in > [1] evaluates to false. The recently merged (my) pci-epf-test code > follows this patter, see [2]. > > The point is that ep_func->epf_bar[bar] only stores a pointer, and the > actual object is owned by the API caller. Since struct pci_epf embeds the > BAR descriptors, updating an epf_bar in place (pattern (b)) seems quite > natural and, in my view, should be supported. (cut) > Requiring callers to always pass a new epf_bar (pattern (a)) feels > contrary to the current API/data-structure design. I disagree. I previously suggested that you should look at pci_epf_test_enable_doorbell() and pci_epf_test_disable_doorbell(). If we look at the workflow in pci-epf-test for the doorbell test case: 1) pci_epf_test_epc_init() calls pci_epf_test_set_bar() which calls pci_epc_set_bar(..., &epf->bar[bar]); 2) pci_epf_test_enable_doorbell() uses a new struct pci_epf_bar (epf_test->db_bar) copies the barno, size and flags for the BAR is will dynamically change: epf_test->db_bar.barno = bar; epf_test->db_bar.size = epf->bar[bar].size; epf_test->db_bar.flags = epf->bar[bar].flags; calls pci_epc_set_bar(..., &epf_test->db_bar); 3) pci_epf_test_disable_doorbell() calls pci_epc_set_bar(..., &epf->bar[bar]); with the original unmodified struct pci_epf_bar. Your new test case however, reuses epf->bar[bar] in step 1, 2, 3. I think the solution is to change your test case so that you use same workflow as the doorbell test case. Just add a new struct pci_epf_bar to struct pci_epf_test where you store the temporary BAR. I'm not a big fan of your suggested code changes. I think adding the early return in dw_pcie_ep_clear_ib_maps() and modifying your test case to use a new epf_bar are the only things that should be fixed as soon as possible. > One downside is that this introduces a behavioral change in a specific > failure case that already existed prior to this series. > > Concretely, this only affects the traditional reprogramming path where a > BAR configured in BAR Match Mode is updated again to BAR Match Mode. If > such an update fails (for example, because the new epf_bar->phys_addr is > not aligned to region_align), the behavior changes as follows: > > - Before this change: the previously programmed BAR Match Mode mapping > remains in place. > - After this change: the existing mapping is torn down first, so no mapping > remains after the failure. > > I don't think this change is a major issue, since the caller will still > observe the failure and should handle the error accordingly in either case. > That said, I'd appreciate feedback if retaining the old BAR Match Mode > mapping on such failure scenario is considered important for the existing > update path. You have found a potential problem with the API though, but like you said, this problem was there even before your changes, so this problem can take more time to be addressed. Since the DWC EP driver only stores pointers to struct pci_epf_bar BARs in set_bar(), it is theoretically possible for the caller to modify this struct after calling set_bar(). This means that any future safety check against values in ep_func->epf_bar (e.g. [1]) is unreliable, as the caller could have modified the struct after calling set_bar(). [1] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L368-L370 I think the solution to this is to modify struct dw_pcie_ep_func: @@ -472,7 +472,7 @@ struct dw_pcie_ep_func { u8 msi_cap; /* MSI capability offset */ u8 msix_cap; /* MSI-X capability offset */ u8 bar_to_atu[PCI_STD_NUM_BARS]; - struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; + struct pci_epf_bar epf_bar[PCI_STD_NUM_BARS]; /* Only for Address Match Mode inbound iATU */ u32 *ib_atu_indexes[PCI_STD_NUM_BARS]; And then modify set_bar() to something like: @@ -588,7 +588,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, if (ret) return ret; - ep_func->epf_bar[bar] = epf_bar; + memcpy(&ep_func->epf_bar[bar], epf_bar, sizeof *epf_bar); return 0; } But that would also mean that we need to modify the if (ep_func->epf_bar[bar]) with something like if (ep_func->epf_bar_in_use[bar]) Or... I guess we could keep it as pointers, but use kmemdup() in set_bar() to get our own immutable copy, and then kfree() the pointer in clear_bar() (and in set_bar() before kmemduping the memory, if the DWC driver already has a local copy (i.e. dynamically changing the BAR without first calling clear_bar())) Kind regards, Niklas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 22:51 ` Niklas Cassel @ 2026-01-31 13:42 ` Koichiro Den 0 siblings, 0 replies; 15+ messages in thread From: Koichiro Den @ 2026-01-31 13:42 UTC (permalink / raw) To: Niklas Cassel Cc: Aksh Garg, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Fri, Jan 30, 2026 at 11:51:33PM +0100, Niklas Cassel wrote: > Hello Koichiro, > > On Sat, Jan 31, 2026 at 02:16:37AM +0900, Koichiro Den wrote: > > While looking at this, I spotted a subtle corner case that the missing > > 'return' has been masking. > > > > Consider the following re-programming sequence for a BAR: > > > > 1) The initial set_bar() programs the BAR in BAR Match Mode > > 2) The second set_bar() switches the BAR to Address Match Mode > > - Ad part of this, set_bar() calls dw_pcie_ep_clear_ib_maps() before > > re-programming. > > 3) The third set_bar() switches back to BAR Match Mode. > > > > In step (3), the current behavior depends on how the caller handles the > > struct pci_epf_bar passed to set_bar() > > > > a) If the caller passes a freshly prepared epf_bar with .num_submap = 0, > > dw_pcie_ep_clear_ib_maps() is called as expected. > > > > b) If the caller updates the same epf_bar in place (i.e. reused the > > object that passed in step (2)) and simply updates .num_submap back to > > 0, dw_pcie_ep_clear_ib_maps() is NOT called because the condition in > > [1] evaluates to false. The recently merged (my) pci-epf-test code > > follows this patter, see [2]. > > > > The point is that ep_func->epf_bar[bar] only stores a pointer, and the > > actual object is owned by the API caller. Since struct pci_epf embeds the > > BAR descriptors, updating an epf_bar in place (pattern (b)) seems quite > > natural and, in my view, should be supported. > > (cut) > > > Requiring callers to always pass a new epf_bar (pattern (a)) feels > > contrary to the current API/data-structure design. > > I disagree. > > I previously suggested that you should look at pci_epf_test_enable_doorbell() > and pci_epf_test_disable_doorbell(). > > If we look at the workflow in pci-epf-test for the doorbell test case: > > 1) > pci_epf_test_epc_init() calls pci_epf_test_set_bar() which calls > pci_epc_set_bar(..., &epf->bar[bar]); > > 2) > pci_epf_test_enable_doorbell() > uses a new struct pci_epf_bar (epf_test->db_bar) > > copies the barno, size and flags for the BAR is will dynamically change: > epf_test->db_bar.barno = bar; > epf_test->db_bar.size = epf->bar[bar].size; > epf_test->db_bar.flags = epf->bar[bar].flags; > calls pci_epc_set_bar(..., &epf_test->db_bar); > > 3) > pci_epf_test_disable_doorbell() calls > pci_epc_set_bar(..., &epf->bar[bar]); > with the original unmodified struct pci_epf_bar. > > > Your new test case however, reuses epf->bar[bar] in step 1, 2, 3. > > I think the solution is to change your test case so that you use same > workflow as the doorbell test case. Just add a new struct pci_epf_bar > to struct pci_epf_test where you store the temporary BAR. > > > I'm not a big fan of your suggested code changes. > > I think adding the early return in dw_pcie_ep_clear_ib_maps() and > modifying your test case to use a new epf_bar are the only things > that should be fixed as soon as possible. > Thanks, that makes sense. I was a bit confused initially, so I'd like to make the API rule explicit in the documentation to avoid further ambiguity. I have just submitted a separate patch series: https://lore.kernel.org/linux-pci/20260131133655.218018-1-den@valinux.co.jp In this series, [PATCH 3/3] adds text describing the caller ownership and lifetime rules for pci_epc_set_bar(). > > > One downside is that this introduces a behavioral change in a specific > > failure case that already existed prior to this series. > > > > Concretely, this only affects the traditional reprogramming path where a > > BAR configured in BAR Match Mode is updated again to BAR Match Mode. If > > such an update fails (for example, because the new epf_bar->phys_addr is > > not aligned to region_align), the behavior changes as follows: > > > > - Before this change: the previously programmed BAR Match Mode mapping > > remains in place. > > - After this change: the existing mapping is torn down first, so no mapping > > remains after the failure. > > > > I don't think this change is a major issue, since the caller will still > > observe the failure and should handle the error accordingly in either case. > > That said, I'd appreciate feedback if retaining the old BAR Match Mode > > mapping on such failure scenario is considered important for the existing > > update path. > > You have found a potential problem with the API though, but like you said, > this problem was there even before your changes, so this problem can take > more time to be addressed. > > Since the DWC EP driver only stores pointers to struct pci_epf_bar BARs > in set_bar(), it is theoretically possible for the caller to modify this > struct after calling set_bar(). This means that any future safety check > against values in ep_func->epf_bar (e.g. [1]) is unreliable, as the caller > could have modified the struct after calling set_bar(). > > [1] https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L368-L370 > > I think the solution to this is to modify struct dw_pcie_ep_func: > > @@ -472,7 +472,7 @@ struct dw_pcie_ep_func { > u8 msi_cap; /* MSI capability offset */ > u8 msix_cap; /* MSI-X capability offset */ > u8 bar_to_atu[PCI_STD_NUM_BARS]; > - struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > + struct pci_epf_bar epf_bar[PCI_STD_NUM_BARS]; > > /* Only for Address Match Mode inbound iATU */ > u32 *ib_atu_indexes[PCI_STD_NUM_BARS]; > > > > And then modify set_bar() to something like: > > @@ -588,7 +588,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > if (ret) > return ret; > > - ep_func->epf_bar[bar] = epf_bar; > + memcpy(&ep_func->epf_bar[bar], epf_bar, sizeof *epf_bar); > > return 0; > } > > > But that would also mean that we need to modify the if (ep_func->epf_bar[bar]) > with something like if (ep_func->epf_bar_in_use[bar]) > > Or... I guess we could keep it as pointers, but use kmemdup() in set_bar() > to get our own immutable copy, and then kfree() the pointer in clear_bar() > (and in set_bar() before kmemduping the memory, if the DWC driver already > has a local copy (i.e. dynamically changing the BAR without first calling > clear_bar())) That feels like the right direction. I'll take a closer look at that, but if you decide to work on it first, please feel free to go ahead. I'd be happy to help with testing. Thank you very much for the careful review and discussion. Kind regards, Koichiro > > > Kind regards, > Niklas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-29 14:14 ` Niklas Cassel 2026-01-30 2:21 ` Koichiro Den @ 2026-01-30 4:12 ` Aksh Garg 2026-01-30 9:53 ` Niklas Cassel 1 sibling, 1 reply; 15+ messages in thread From: Aksh Garg @ 2026-01-30 4:12 UTC (permalink / raw) To: Niklas Cassel, Koichiro Den Cc: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On 29/01/26 19:44, Niklas Cassel wrote: > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: >> -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) >> { >> + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); >> struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >> struct device *dev = pci->dev; >> unsigned int i, num; >> @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> u32 *indexes; > > Hello Aksh, > > Considering that all other functions that you have modified, you have added a: > > if (!ep_func) > return; > > > I think you should do the same to this function. > I omitted this NULL check here because all the current call sites of dw_pcie_ep_clear_ib_maps() already perform this validation. I felt adding it here would add redundancy in the code. > >> >> /* Tear down the BAR Match Mode mapping, if any. */ >> - if (ep->bar_to_atu[bar]) { >> - atu_index = ep->bar_to_atu[bar] - 1; >> + if (ep_func->bar_to_atu[bar]) { >> + atu_index = ep_func->bar_to_atu[bar] - 1; >> dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index); >> clear_bit(atu_index, ep->ib_window_map); >> - ep->bar_to_atu[bar] = 0; >> + ep_func->bar_to_atu[bar] = 0; ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 4:12 ` Aksh Garg @ 2026-01-30 9:53 ` Niklas Cassel 2026-01-30 10:39 ` Aksh Garg 0 siblings, 1 reply; 15+ messages in thread From: Niklas Cassel @ 2026-01-30 9:53 UTC (permalink / raw) To: Aksh Garg Cc: Koichiro Den, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Fri, Jan 30, 2026 at 09:42:43AM +0530, Aksh Garg wrote: > On 29/01/26 19:44, Niklas Cassel wrote: > > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: > > > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) > > > { > > > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > struct device *dev = pci->dev; > > > unsigned int i, num; > > > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > > u32 *indexes; > > > > Hello Aksh, > > > > Considering that all other functions that you have modified, you have added a: > > > > if (!ep_func) > > return; > > > > > > I think you should do the same to this function. > > > > I omitted this NULL check here because all the current call sites of > dw_pcie_ep_clear_ib_maps() already perform this validation. I felt > adding it here would add redundancy in the code. Ok, but with that logic, shouldn't we also remove the NULL checks from dw_pcie_ep_ib_atu_bar() and dw_pcie_ep_ib_atu_addr(), because they are only called from dw_pcie_ep_set_bar(), which already has the ep_func NULL check? Kind regards, Niklas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 9:53 ` Niklas Cassel @ 2026-01-30 10:39 ` Aksh Garg 2026-01-30 10:47 ` Niklas Cassel 0 siblings, 1 reply; 15+ messages in thread From: Aksh Garg @ 2026-01-30 10:39 UTC (permalink / raw) To: Niklas Cassel Cc: Koichiro Den, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On 30/01/26 15:23, Niklas Cassel wrote: > On Fri, Jan 30, 2026 at 09:42:43AM +0530, Aksh Garg wrote: >> On 29/01/26 19:44, Niklas Cassel wrote: >> > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: >> > > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> > > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) >> > > { >> > > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); >> > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >> > > struct device *dev = pci->dev; >> > > unsigned int i, num; >> > > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> > > u32 *indexes; >> > >> > Hello Aksh, >> > >> > Considering that all other functions that you have modified, you have added a: >> > >> > if (!ep_func) >> > return; >> > >> > >> > I think you should do the same to this function. >> > >> >> I omitted this NULL check here because all the current call sites of >> dw_pcie_ep_clear_ib_maps() already perform this validation. I felt >> adding it here would add redundancy in the code. > > Ok, but with that logic, shouldn't we also remove the NULL checks from > dw_pcie_ep_ib_atu_bar() and dw_pcie_ep_ib_atu_addr(), because they are > only called from dw_pcie_ep_set_bar(), which already has the ep_func > NULL check? > Yes, that's correct. Alternatively, we can add the NULL check in dw_pcie_ep_clear_ib_maps() as well, making all the functions using ep_func self-contained and defensive, removing the dependency on whether the callers perform NULL checks. This makes the code more future proof, as new callers won't need to be aware of the NULL pointer possibility. Please provide your views on this approach. > > Kind regards, > Niklas > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 10:39 ` Aksh Garg @ 2026-01-30 10:47 ` Niklas Cassel 2026-01-30 10:52 ` Aksh Garg 0 siblings, 1 reply; 15+ messages in thread From: Niklas Cassel @ 2026-01-30 10:47 UTC (permalink / raw) To: Aksh Garg Cc: Koichiro Den, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On Fri, Jan 30, 2026 at 04:09:26PM +0530, Aksh Garg wrote: > On 30/01/26 15:23, Niklas Cassel wrote: > > On Fri, Jan 30, 2026 at 09:42:43AM +0530, Aksh Garg wrote: > > > On 29/01/26 19:44, Niklas Cassel wrote: > > > > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: > > > > > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > > > > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) > > > > > { > > > > > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > > struct device *dev = pci->dev; > > > > > unsigned int i, num; > > > > > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) > > > > > u32 *indexes; > > > > > Hello Aksh, > > > > > Considering that all other functions that you have modified, you > > > have added a: > > > > > if (!ep_func) > > > > return; > > > > > > I think you should do the same to this function. > > > > > > > > > > I omitted this NULL check here because all the current call sites of > > > dw_pcie_ep_clear_ib_maps() already perform this validation. I felt > > > adding it here would add redundancy in the code. > > > > Ok, but with that logic, shouldn't we also remove the NULL checks from > > dw_pcie_ep_ib_atu_bar() and dw_pcie_ep_ib_atu_addr(), because they are > > only called from dw_pcie_ep_set_bar(), which already has the ep_func > > NULL check? > > > > Yes, that's correct. Alternatively, we can add the NULL check in > dw_pcie_ep_clear_ib_maps() as well, making all the functions using > ep_func self-contained and defensive, removing the dependency on whether > the callers perform NULL checks. This makes the code more future proof, > as new callers won't need to be aware of the NULL pointer possibility. Sounds like a good idea to me. Neither of these functions is in the hot path, so the performance of having one less NULL check is not super critical. Kind regards, Niklas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support 2026-01-30 10:47 ` Niklas Cassel @ 2026-01-30 10:52 ` Aksh Garg 0 siblings, 0 replies; 15+ messages in thread From: Aksh Garg @ 2026-01-30 10:52 UTC (permalink / raw) To: Niklas Cassel Cc: Koichiro Den, linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, Zhiqiang.Hou, gustavo.pimentel, linux-kernel, s-vadapalli, danishanwar On 30/01/26 16:17, Niklas Cassel wrote: > On Fri, Jan 30, 2026 at 04:09:26PM +0530, Aksh Garg wrote: >> On 30/01/26 15:23, Niklas Cassel wrote: >> > On Fri, Jan 30, 2026 at 09:42:43AM +0530, Aksh Garg wrote: >> > > On 29/01/26 19:44, Niklas Cassel wrote: >> > > > On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote: >> > > > > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> > > > > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar) >> > > > > { >> > > > > + struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); >> > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >> > > > > struct device *dev = pci->dev; >> > > > > unsigned int i, num; >> > > > > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar) >> > > > > u32 *indexes; >> > > > > Hello Aksh, >> > > > > Considering that all other functions that you have modified, you >> > > have added a: >> > > > > if (!ep_func) >> > > > return; >> > > > > > I think you should do the same to this function. >> > > > >> > > >> > > I omitted this NULL check here because all the current call sites of >> > > dw_pcie_ep_clear_ib_maps() already perform this validation. I felt >> > > adding it here would add redundancy in the code. >> > >> > Ok, but with that logic, shouldn't we also remove the NULL checks from >> > dw_pcie_ep_ib_atu_bar() and dw_pcie_ep_ib_atu_addr(), because they are >> > only called from dw_pcie_ep_set_bar(), which already has the ep_func >> > NULL check? >> > >> >> Yes, that's correct. Alternatively, we can add the NULL check in >> dw_pcie_ep_clear_ib_maps() as well, making all the functions using >> ep_func self-contained and defensive, removing the dependency on whether >> the callers perform NULL checks. This makes the code more future proof, >> as new callers won't need to be aware of the NULL pointer possibility. > > Sounds like a good idea to me. > > Neither of these functions is in the hot path, so the performance of having > one less NULL check is not super critical. > Okay, I will post a new patch series adding the NULL check in the function. Regards, Aksh Garg > > Kind regards, > Niklas > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] PCI: dwc: ep: Add comment explaining controller-level PTM access 2026-01-29 9:17 [PATCH v4 0/3] PCI: dwc: ep: Enhance multi-function endpoint support Aksh Garg 2026-01-29 9:17 ` [PATCH v4 1/3] PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations Aksh Garg 2026-01-29 9:17 ` [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support Aksh Garg @ 2026-01-29 9:17 ` Aksh Garg 2 siblings, 0 replies; 15+ messages in thread From: Aksh Garg @ 2026-01-29 9:17 UTC (permalink / raw) To: linux-pci, jingoohan1, mani, lpieralisi, kwilczynski, robh, bhelgaas, cassel, Zhiqiang.Hou, gustavo.pimentel Cc: linux-kernel, s-vadapalli, danishanwar, Aksh Garg PCIe r6.0, section 7.9.15 requires PTM capability in exactly one function to control all PTM-capable functions. This makes PTM registers controller-level rather than per-function. Add a comment explaining why PTM capability registers are accessed using the standard DBI accessors instead of func_no indexed per-function accessors. Suggested-by: Niklas Cassel <cassel@kernel.org> Signed-off-by: Aksh Garg <a-garg7@ti.com> Reviewed-by: Niklas Cassel <cassel@kernel.org> --- Changes from v3 to v4: - Rebase Changes from v2 to v3: - Fixed the suggested nits v3: https://lore.kernel.org/all/20260127085010.446116-4-a-garg7@ti.com/ v2: https://lore.kernel.org/all/20260122082538.309122-4-a-garg7@ti.com/ drivers/pci/controller/dwc/pcie-designware-ep.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index b9a234b47aab..aa0c1dac297c 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -1183,6 +1183,16 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) if (ep->ops->init) ep->ops->init(ep); + /* + * PCIe r6.0, section 7.9.15 states that for endpoints that support PTM, + * this capability structure is required in exactly one function, which + * controls the PTM behavior of all PTM capable functions. This indicates + * the PTM capability structure represents controller-level registers + * rather than per-function registers. + * + * Therefore, PTM capability registers are configured using the standard DBI + * accessors, instead of func_no indexed per-function accessors. + */ ptm_cap_base = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM); /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-01-31 13:42 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-29 9:17 [PATCH v4 0/3] PCI: dwc: ep: Enhance multi-function endpoint support Aksh Garg 2026-01-29 9:17 ` [PATCH v4 1/3] PCI: dwc: ep: Fix resizable BAR support for multi-PF configurations Aksh Garg 2026-01-29 9:17 ` [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support Aksh Garg 2026-01-29 14:14 ` Niklas Cassel 2026-01-30 2:21 ` Koichiro Den 2026-01-30 9:57 ` Niklas Cassel 2026-01-30 17:16 ` Koichiro Den 2026-01-30 22:51 ` Niklas Cassel 2026-01-31 13:42 ` Koichiro Den 2026-01-30 4:12 ` Aksh Garg 2026-01-30 9:53 ` Niklas Cassel 2026-01-30 10:39 ` Aksh Garg 2026-01-30 10:47 ` Niklas Cassel 2026-01-30 10:52 ` Aksh Garg 2026-01-29 9:17 ` [PATCH v4 3/3] PCI: dwc: ep: Add comment explaining controller-level PTM access Aksh Garg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox