The BAR subrange setup/clear paths in pci-epf-test used to update
epf->bar[barno].submap in place and free/restore the submap around
pci_epc_set_bar() calls.

Some EPC drivers may keep a reference to the struct pci_epf_bar passed
to pci_epc_set_bar(). Mutating or freeing the same bar descriptor after
a successful set_bar() can therefore lead to unexpected behaviour.

Use a dedicated pci_epf_bar instance for the subrange mapping test and
only free the allocated submap after restoring the BAR mapping back to
the default epf->bar[barno] descriptor.

Fixes: 6c5e6101423b ("PCI: endpoint: pci-epf-test: Add BAR subrange mapping test support")
Suggested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 32 ++++++-------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 6952ee418622..fd6452d1dcc7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -86,6 +86,7 @@ struct pci_epf_test {
 	bool			dma_private;
 	const struct pci_epc_features *epc_features;
 	struct pci_epf_bar	db_bar;
+	struct pci_epf_bar	subrange_bar[PCI_STD_NUM_BARS];
 	size_t			bar_size[PCI_STD_NUM_BARS];
 };
 
@@ -827,11 +828,11 @@ static u8 pci_epf_test_subrange_sig_byte(enum pci_barno barno,
 static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 					    struct pci_epf_test_reg *reg)
 {
-	struct pci_epf_bar_submap *submap, *old_submap;
+	struct pci_epf_bar_submap *submap;
 	struct pci_epf *epf = epf_test->epf;
 	struct pci_epc *epc = epf->epc;
 	struct pci_epf_bar *bar;
-	unsigned int nsub = PCI_EPF_TEST_BAR_SUBRANGE_NSUB, old_nsub;
+	unsigned int nsub = PCI_EPF_TEST_BAR_SUBRANGE_NSUB;
 	/* reg->size carries BAR number for BAR_SUBRANGE_* commands. */
 	enum pci_barno barno = le32_to_cpu(reg->size);
 	u32 status = le32_to_cpu(reg->status);
@@ -857,7 +858,8 @@ static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 		goto err;
 	}
 
-	bar = &epf->bar[barno];
+	bar = &epf_test->subrange_bar[barno];
+	*bar = epf->bar[barno];
 	if (!bar->size || !bar->addr) {
 		dev_err(&epf->dev, "bar size/addr (%zu/%p) is invalid\n",
 			bar->size, bar->addr);
@@ -883,21 +885,17 @@ static void pci_epf_test_bar_subrange_setup(struct pci_epf_test *epf_test,
 		submap[i].size = sub_size;
 	}
 
-	old_submap = bar->submap;
-	old_nsub = bar->num_submap;
-
 	bar->submap = submap;
 	bar->num_submap = nsub;
 
 	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, bar);
 	if (ret) {
 		dev_err(&epf->dev, "pci_epc_set_bar() failed: %d\n", ret);
-		bar->submap = old_submap;
-		bar->num_submap = old_nsub;
+		bar->submap = NULL;
+		bar->num_submap = 0;
 		kfree(submap);
 		goto err;
 	}
-	kfree(old_submap);
 
 	/*
 	 * Fill deterministic signatures into the physical regions that
@@ -925,13 +923,11 @@ static void pci_epf_test_bar_subrange_clear(struct pci_epf_test *epf_test,
 					    struct pci_epf_test_reg *reg)
 {
 	struct pci_epf *epf = epf_test->epf;
-	struct pci_epf_bar_submap *submap;
 	struct pci_epc *epc = epf->epc;
 	/* reg->size carries BAR number for BAR_SUBRANGE_* commands. */
 	enum pci_barno barno = le32_to_cpu(reg->size);
 	u32 status = le32_to_cpu(reg->status);
 	struct pci_epf_bar *bar;
-	unsigned int nsub;
 	int ret;
 
 	if (barno >= PCI_STD_NUM_BARS) {
@@ -940,23 +936,15 @@ static void pci_epf_test_bar_subrange_clear(struct pci_epf_test *epf_test,
 	}
 
 	bar = &epf->bar[barno];
-	submap = bar->submap;
-	nsub = bar->num_submap;
-
-	if (!submap || !nsub)
-		goto err;
-
-	bar->submap = NULL;
-	bar->num_submap = 0;
 
 	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, bar);
 	if (ret) {
-		bar->submap = submap;
-		bar->num_submap = nsub;
 		dev_err(&epf->dev, "pci_epc_set_bar() failed: %d\n", ret);
 		goto err;
 	}
-	kfree(submap);
+	kfree(epf_test->subrange_bar[barno].submap);
+	epf_test->subrange_bar[barno].submap = NULL;
+	epf_test->subrange_bar[barno].num_submap = 0;
 
 	status |= STATUS_BAR_SUBRANGE_CLEAR_SUCCESS;
 	reg->status = cpu_to_le32(status);
-- 
2.51.0

On Sat, Jan 31, 2026 at 10:36:54PM +0900, Koichiro Den wrote:
> The BAR subrange setup/clear paths in pci-epf-test used to update
> epf->bar[barno].submap in place and free/restore the submap around
> pci_epc_set_bar() calls.
>
> Some EPC drivers may keep a reference to the struct pci_epf_bar passed
> to pci_epc_set_bar(). Mutating or freeing the same bar descriptor after
> a successful set_bar() can therefore lead to unexpected behaviour.
>
> Use a dedicated pci_epf_bar instance for the subrange mapping test and
> only free the allocated submap after restoring the BAR mapping back to
> the default epf->bar[barno] descriptor.
>
> Fixes: 6c5e6101423b ("PCI: endpoint: pci-epf-test: Add BAR subrange mapping test support")
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 32 ++++++-------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 6952ee418622..fd6452d1dcc7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -86,6 +86,7 @@ struct pci_epf_test {
>	bool			dma_private;
>	const struct pci_epc_features *epc_features;
>	struct pci_epf_bar	db_bar;
> +	struct pci_epf_bar	subrange_bar[PCI_STD_NUM_BARS];


If we compare your test:
pci_epf_test_bar_subrange_setup(), the host side decides which BAR you
want to configure.

For pci_epf_test_enable_doorbell(), the function itself uses
pci_epc_get_next_free_bar(), so the EP side decides which BAR to use.

This is a difference, but I think your way is fine.



Another difference is that you have:
struct pci_epf_bar      subrange_bar[PCI_STD_NUM_BARS];

while the doorbell test case has:

struct pci_epf_bar      db_bar;

Looking at the code, you allow multiple BARs to be configured in subrange
mapping mode (even though the selftest itself will only enable+disable it
for one BAR one by one, but I guess someone could theoretically write their
own test program that puts all the BARs in subrange mapping mode at the same
time).

This is another difference from enable_doorbell(), but again I think your
way is also fine.



Looking at the pci-epf-test code, I realize that, because:
struct pci_epf_bar      db_bar;
is just a single struct, doing ioctl ENABLE_DOORBELL multiple times will
just overwrite the existing db_bar struct... Not very nice...

Since it is only one db_bar, pci-epf-test should return an error if
ENABLE_DOORBELL is called multiple times in a row, rather than just silently
overwrite pci_epf_bar db_bar, leaving the previous BAR still configured
while programming yet another BAR for a HW doorbell... This means that
calling DISABLE_DOORBELL will incorrectly just cleanup one BAR and not two...
This is not your bug however...



TL;DR: I think your code looks fine, even though it is different that
the doorbell test case in a few ways.

Also the doorbell test case is buggy, but that is not really your problem.


Kind regards,
Niklas