Re: [PATCH v3 5/9] PCI: endpoint: pci-epf-test: Simplify pci_epf_test_set_bar() loop

[Niklas Cassel <cassel@kernel.org>]

On Fri, Mar 15, 2024 at 11:09:03AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:57AM +0100, Niklas Cassel wrote:
> > Simplify the loop in pci_epf_test_set_bar().
> > If we allocated memory for the BAR, we need to call set_bar() for that
> > BAR, if we did not allocated memory for that BAR, we need to skip.
> > It is as simple as that. This also matches the logic in
> > pci_epf_test_unbind().
> > 
> > A 64-bit BAR will still only be one allocation, with the BAR succeeding
> > the 64-bit BAR being null.
> > 
> > While at it, remove the misleading comment.
> > A EPC .set_bar() callback should never change the epf_bar->flags.
> > (E.g. to set a 64-bit BAR if we requested a 32-bit BAR.)
> > 
> > A .set_bar() callback should do what we request it to do.
> > If it can't satisfy the request, it should return an error.
> > 
> 
> That's a valid assumption. But...
> 
> > If platform has a specific requirement, e.g. that a certain BAR has to
> > be a 64-bit BAR, then it should specify that by setting the .only_64bit
> > flag for that specific BAR in epc_features->bar[], such that
> > pci_epf_alloc_space() will return a epf_bar with the 64-bit flag set.
> > (Such that .set_bar() will receive a request to set a 64-bit BAR.)
> > 
> 
> Looks like pcie-cadence-ep is setting the PCI_BASE_ADDRESS_MEM_TYPE_64 flag if
> the requested size is >2GB (I don't know why 2GB instead of 4GB in the first
> place).

That is dead code that will never be able to execute.

Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
is invalid if 64-bit flag is not set") it has been impossible to get the
.set_bar() callback with a BAR size > 2 GB.

Yes, 2GB!
The author of f25b5fae29d4 is an idiot (yes, it is me).
Anyway, the code itself in that commit is doing the right thing...

2GB is the maximum size of a 32-bit BAR.

So, since the the code in pcie-cadence-ep is dead code, I don't see
any problem with this commit.


Kind regards,
Niklas


> 
> - Mani
> 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++---------------
> >  1 file changed, 4 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 20c79610712d..91bbfcb1b3ed 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -709,31 +709,18 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >  
> >  static int pci_epf_test_set_bar(struct pci_epf *epf)
> >  {
> > -	int bar, add;
> > -	int ret;
> > -	struct pci_epf_bar *epf_bar;
> > +	int bar, ret;
> >  	struct pci_epc *epc = epf->epc;
> >  	struct device *dev = &epf->dev;
> >  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> >  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> > -	const struct pci_epc_features *epc_features;
> > -
> > -	epc_features = epf_test->epc_features;
> >  
> > -	for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
> > -		epf_bar = &epf->bar[bar];
> > -		/*
> > -		 * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> > -		 * if the specific implementation required a 64-bit BAR,
> > -		 * even if we only requested a 32-bit BAR.
> > -		 */
> > -		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
> > -
> > -		if (epc_features->bar[bar].type == BAR_RESERVED)
> > +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +		if (!epf_test->reg[bar])
> >  			continue;
> >  
> >  		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
> > -				      epf_bar);
> > +				      &epf->bar[bar]);
> >  		if (ret) {
> >  			pci_epf_free_space(epf, epf_test->reg[bar], bar,
> >  					   PRIMARY_INTERFACE);
> > -- 
> > 2.44.0
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்