Re: [PATCH] PCI: endpoint: pci-epf-test: Allow overriding default BAR sizes

[Niklas Cassel <cassel@kernel.org>]

Hello Mani,

On Wed, Jan 28, 2026 at 01:47:45PM +0530, Manivannan Sadhasivam wrote:
> > +
> > +	# grep . functions/pci_epf_test/func1/pci_epf_test.0/bar?_size
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar0_size:512
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar1_size:512
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar2_size:1024
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar3_size:16384
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar4_size:131072
> > +	  functions/pci_epf_test/func1/pci_epf_test.0/bar5_size:1048576
> > +
> > +The user can override a default value using e.g.::
> > +	# echo 1048576 > functions/pci_epf_test/func1/pci_epf_test.0/bar1_size
> > +
> > +Note: Some endpoint controllers might have fixed size BARs, or reserved BARs,
> > +for such controllers, the corresponding BAR size in configfs will be ignored.
> > +
> 
> Silently ignoring the BAR size would create confusion. We should error out with
> -EOPNOTSUPP if the BAR is a fixed size BAR.

My initial idea was indeed to return -EOPNOTSUPP if the BAR is a fixed size BAR.
The problem however is that the information if a BAR is FIXED or not is in
epf->epc->features. epf->epc is NULL until the EPF has been bound to an EPC via
configfs.


> 
> >  
> >  Binding pci-epf-test Device to EP Controller
> >  --------------------------------------------
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index b785d488c284..fda257e46920 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -63,6 +63,7 @@ static struct workqueue_struct *kpcitest_workqueue;
> >  struct pci_epf_test {
> >  	void			*reg[PCI_STD_NUM_BARS];
> >  	struct pci_epf		*epf;
> > +	struct config_group	group;
> >  	enum pci_barno		test_reg_bar;
> >  	size_t			msix_table_offset;
> >  	struct delayed_work	cmd_handler;
> > @@ -76,6 +77,7 @@ struct pci_epf_test {
> >  	bool			dma_private;
> >  	const struct pci_epc_features *epc_features;
> >  	struct pci_epf_bar	db_bar;
> > +	size_t			bar_size[PCI_STD_NUM_BARS];
> >  };
> >  
> >  struct pci_epf_test_reg {
> > @@ -102,7 +104,8 @@ static struct pci_epf_header test_header = {
> >  	.interrupt_pin	= PCI_INTERRUPT_INTA,
> >  };
> >  
> > -static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> > +/* default BAR sizes, can be overridden by the user using configfs */
> > +static size_t default_bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> >  
> >  static void pci_epf_test_dma_callback(void *param)
> >  {
> > @@ -1070,7 +1073,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >  		if (epc_features->bar[bar].type == BAR_FIXED)
> >  			test_reg_size = epc_features->bar[bar].fixed_size;
> >  		else
> > -			test_reg_size = bar_size[bar];
> > +			test_reg_size = epf_test->bar_size[bar];
> >  
> >  		base = pci_epf_alloc_space(epf, test_reg_size, bar,
> >  					   epc_features, PRIMARY_INTERFACE);
> > @@ -1142,6 +1145,87 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >  	pci_epf_test_free_space(epf);
> >  }
> >  
> > +#define PCI_EPF_TEST_BAR_SIZE_R(_name, _id)				\
> > +	static ssize_t pci_epf_test_##_name##_show(struct config_item *item,	\
> > +						   char *page)		\
> > +	{								\
> > +		struct config_group *group = to_config_group(item);	\
> > +		struct pci_epf_test *epf_test = container_of(group,	\
> > +					struct pci_epf_test, group);	\
> > +									\
> > +		return sprintf(page, "%zu\n", epf_test->bar_size[_id]);	\
> > +	}
> > +
> > +#define PCI_EPF_TEST_BAR_SIZE_W(_name, _id)				\
> > +	static ssize_t pci_epf_test_##_name##_store(struct config_item *item, \
> > +						    const char *page, size_t len) \
> > +	{								\
> > +		struct config_group *group = to_config_group(item);	\
> > +		struct pci_epf_test *epf_test = container_of(group,	\
> > +					struct pci_epf_test, group);	\
> > +		int val;						\
> > +		int ret;						\
> > +									\
> > +		ret = kstrtouint(page, 0, &val);			\
> > +		if (ret)						\
> > +			return ret;					\
> > +									\
> > +		if (!is_power_of_2(val))				\
> > +			return -EINVAL;					\
> > +									\
> > +		epf_test->bar_size[_id] = val;				\
> > +									\
> > +		return len;						\
> > +	}
> > +
> > +PCI_EPF_TEST_BAR_SIZE_R(bar0_size, BAR_0)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar0_size, BAR_0)
> > +PCI_EPF_TEST_BAR_SIZE_R(bar1_size, BAR_1)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar1_size, BAR_1)
> > +PCI_EPF_TEST_BAR_SIZE_R(bar2_size, BAR_2)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar2_size, BAR_2)
> > +PCI_EPF_TEST_BAR_SIZE_R(bar3_size, BAR_3)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar3_size, BAR_3)
> > +PCI_EPF_TEST_BAR_SIZE_R(bar4_size, BAR_4)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar4_size, BAR_4)
> > +PCI_EPF_TEST_BAR_SIZE_R(bar5_size, BAR_5)
> > +PCI_EPF_TEST_BAR_SIZE_W(bar5_size, BAR_5)
> > +
> > +CONFIGFS_ATTR(pci_epf_test_, bar0_size);
> > +CONFIGFS_ATTR(pci_epf_test_, bar1_size);
> > +CONFIGFS_ATTR(pci_epf_test_, bar2_size);
> > +CONFIGFS_ATTR(pci_epf_test_, bar3_size);
> > +CONFIGFS_ATTR(pci_epf_test_, bar4_size);
> > +CONFIGFS_ATTR(pci_epf_test_, bar5_size);
> > +
> > +static struct configfs_attribute *pci_epf_test_attrs[] = {
> > +	&pci_epf_test_attr_bar0_size,
> > +	&pci_epf_test_attr_bar1_size,
> > +	&pci_epf_test_attr_bar2_size,
> > +	&pci_epf_test_attr_bar3_size,
> > +	&pci_epf_test_attr_bar4_size,
> > +	&pci_epf_test_attr_bar5_size,
> > +	NULL,
> > +};
> > +
> > +static const struct config_item_type pci_epf_test_group_type = {
> > +	.ct_attrs	= pci_epf_test_attrs,
> > +	.ct_owner	= THIS_MODULE,
> > +};
> > +
> > +static struct config_group *pci_epf_test_add_cfs(struct pci_epf *epf,
> > +						 struct config_group *group)
> > +{
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	struct config_group *epf_group = &epf_test->group;
> > +	struct device *dev = &epf->dev;
> > +
> > +	config_group_init_type_name(epf_group, dev_name(dev),
> > +				    &pci_epf_test_group_type);
> > +
> > +	return epf_group;
> > +}
> > +
> >  static const struct pci_epf_device_id pci_epf_test_ids[] = {
> >  	{
> >  		.name = "pci_epf_test",
> > @@ -1154,6 +1238,7 @@ static int pci_epf_test_probe(struct pci_epf *epf,
> >  {
> >  	struct pci_epf_test *epf_test;
> >  	struct device *dev = &epf->dev;
> > +	enum pci_barno bar;
> >  
> >  	epf_test = devm_kzalloc(dev, sizeof(*epf_test), GFP_KERNEL);
> >  	if (!epf_test)
> > @@ -1161,6 +1246,8 @@ static int pci_epf_test_probe(struct pci_epf *epf,
> >  
> >  	epf->header = &test_header;
> >  	epf_test->epf = epf;
> > +	for (bar = BAR_0; bar < PCI_STD_NUM_BARS; bar++)
> > +		epf_test->bar_size[bar] = default_bar_size[bar];
> >  
> 
> It'd be more clean if 'epf_test->bar_size[bar]' always holds the actual BAR size
> i.e., one of default/user configured/fixed size. This will allow us to print the
> actual BAR size used for testing. For this, the initial size setting should
> happen in pci_epf_test_bind().

# mkdir functions/pci_epf_test/func1
will cause pci_epf_test_probe() to be called.

At this point, when the EPF has been created, but not bound to an EPC,
the user could still do:

# grep . functions/pci_epf_test/func1/pci_epf_test.0/bar?_size

If we want this to show something other than e.g. size 0 for all BAR
before binding to and EPC, then we thus need to initialize it in probe().

If you are happy with showing sizes as zero, and that the configfs writes
that a user made to bar*_size before binding to a EPC will be overwritten
(I guess we could also return -EINVAL if bar*_size is written before an EPC
has been bound to the EPF), the next problem is that:

# ln -s functions/pci_epf_test/func1 controllers/a40000000.pcie-ep/
[  578.073152] pci_epf_test_bind
[  578.075614] pci_epf_test_epc_init

will cause both pci_epf_test_bind and pci_epf_test_epc_init to be called.
(for all platforms expect those that can only work with external reference
clock).

As you know, pci_epf_test_epc_init() will call set_bar().

So, the suggestion in this patch is the most pragmatic way I could think of.


The only better solution would be to move all the set_bar() code (and probably
all code) from pci_epf_test_epc_init() to

# echo 1 > controllers/a40000000.pcie-ep/start

time.

But AFAICT, we do not have any callback in an EPF driver that gets called
before the link is started.

I guess we could theoretically add a new EPF callback which pci-epc-core.c
calls for each EPF driver, before calling epc->ops->start(epc); :

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 068155819c57..dc717c5fd195 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -180,6 +180,7 @@ EXPORT_SYMBOL_GPL(pci_epc_stop);
  */
 int pci_epc_start(struct pci_epc *epc)
 {
+       struct pci_epf *epf;
        int ret;
 
        if (IS_ERR(epc))
@@ -188,6 +189,14 @@ int pci_epc_start(struct pci_epc *epc)
        if (!epc->ops->start)
                return 0;
 
+       mutex_lock(&epc->list_lock);
+       list_for_each_entry(epf, &epc->pci_epf, list) {
+               mutex_lock(&epf->lock);
+               if (epf->event_ops && epf->event_ops->epc_start_link)
+                       epf->event_ops->epc_start_link(epf);
+               mutex_unlock(&epf->lock);
+       }
+
        mutex_lock(&epc->lock);
        ret = epc->ops->start(epc);
        mutex_unlock(&epc->lock);


I guess it should work.

But I am slightly worried of the complexity it brings...
Because we already have an .epc_init callback in an EPF driver.
If we also bring add a ".pre_start_link" callback in an EPF driver,
how should an EPF driver developer know what to put in each callback?

I guess in that case we would want to remove the .epc_init callback?

(Because calling .set_bar() etc. should work to be called in a
.pre_start_link for both those driver relying on external refclock and
for those that don't.

If you do
# echo 1 > controllers/a40000000.pcie-ep/start
on a platform that requires an external reflock, before the external
refclock is available, that is your own fault.

I guess we could detect that and give a nice error.)


Kind regards,
Niklas