diff mbox series

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

Message ID 20240313105804.100168-6-cassel@kernel.org (mailing list archive)
State Superseded
Headers show
Series PCI: endpoint: set prefetchable bit for 64-bit BARs | expand

Commit Message

Niklas Cassel March 13, 2024, 10:57 a.m. UTC 
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.

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.)

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++---------------
 1 file changed, 4 insertions(+), 17 deletions(-)

Comments

Manivannan Sadhasivam March 15, 2024, 5:39 a.m. UTC | #1 
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).

- 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
>
Niklas Cassel March 20, 2024, 11:08 a.m. UTC | #2 
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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

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);