[PATCH 0/3] pci_epf_alloc_space() cleanups

[PATCH 0/3] pci_epf_alloc_space() cleanups

[Niklas Cassel]

Hello all,

Here comes some cleanups related to pci_epf_alloc_space().

Kind regards,
Niklas


Niklas Cassel (3):
  PCI: endpoint: refactor pci_epf_alloc_space()
  PCI: endpoint: improve pci_epf_alloc_space()
  PCI: endpoint: pci-epf-vntb: remove superfluous checks

 drivers/pci/endpoint/functions/pci-epf-ntb.c  |  2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c | 13 ++-----------
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 15 ++-------------
 drivers/pci/endpoint/pci-epf-core.c           | 16 +++++++++++++---
 include/linux/pci-epf.h                       |  4 +++-
 5 files changed, 21 insertions(+), 29 deletions(-)

-- 
2.43.0

[PATCH 1/3] PCI: endpoint: refactor pci_epf_alloc_space()

[Niklas Cassel]

Refactor pci_epf_alloc_space() to take epc_features as a parameter.
This is a preparation patch needed for further cleanups.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-ntb.c  | 2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 4 ++--
 drivers/pci/endpoint/pci-epf-core.c           | 6 ++++--
 include/linux/pci-epf.h                       | 4 +++-
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
index 0553946005c4..43cd309ce22f 100644
--- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
@@ -1067,7 +1067,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb,
 	else if (size < ctrl_size + spad_size)
 		return -EINVAL;
 
-	base = pci_epf_alloc_space(epf, size, barno, align, type);
+	base = pci_epf_alloc_space(epf, size, barno, epc_features, type);
 	if (!base) {
 		dev_err(dev, "%s intf: Config/Status/SPAD alloc region fail\n",
 			pci_epc_interface_string(type));
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 18c80002d3bd..15bfa7d83489 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -848,7 +848,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	}
 
 	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
-				   epc_features->align, PRIMARY_INTERFACE);
+				   epc_features, PRIMARY_INTERFACE);
 	if (!base) {
 		dev_err(dev, "Failed to allocated register space\n");
 		return -ENOMEM;
@@ -866,8 +866,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 			continue;
 
 		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
-					   epc_features->align,
-					   PRIMARY_INTERFACE);
+					   epc_features, PRIMARY_INTERFACE);
 		if (!base)
 			dev_err(dev, "Failed to allocate space for BAR%d\n",
 				bar);
diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index e75a2af77328..ba509d67188b 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -446,7 +446,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	else if (size < ctrl_size + spad_size)
 		return -EINVAL;
 
-	base = pci_epf_alloc_space(epf, size, barno, align, 0);
+	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
 	if (!base) {
 		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
 		return -ENOMEM;
@@ -550,7 +550,7 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 
 	barno = ntb->epf_ntb_bar[BAR_DB];
 
-	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
+	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
 	if (!mw_addr) {
 		dev_err(dev, "Failed to allocate OB address\n");
 		return -ENOMEM;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 2c32de667937..e44f4078fe8b 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -251,14 +251,16 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
  * @epf: the EPF device to whom allocate the memory
  * @size: the size of the memory that has to be allocated
  * @bar: the BAR number corresponding to the allocated register space
- * @align: alignment size for the allocation region
+ * @epc: the features provided by the EPC specific to this endpoint function
  * @type: Identifies if the allocation is for primary EPC or secondary EPC
  *
  * Invoke to allocate memory for the PCI EPF register space.
  */
 void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
-			  size_t align, enum pci_epc_interface_type type)
+			  const struct pci_epc_features *epc_features,
+			  enum pci_epc_interface_type type)
 {
+	size_t align = epc_features->align;
 	struct pci_epf_bar *epf_bar;
 	dma_addr_t phys_addr;
 	struct pci_epc *epc;
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 77b146e0f672..adee6a1b35db 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -15,6 +15,7 @@
 #include <linux/pci.h>
 
 struct pci_epf;
+struct pci_epc_features;
 enum pci_epc_interface_type;
 
 enum pci_barno {
@@ -216,7 +217,8 @@ int __pci_epf_register_driver(struct pci_epf_driver *driver,
 			      struct module *owner);
 void pci_epf_unregister_driver(struct pci_epf_driver *driver);
 void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
-			  size_t align, enum pci_epc_interface_type type);
+			  const struct pci_epc_features *epc_features,
+			  enum pci_epc_interface_type type);
 void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
 			enum pci_epc_interface_type type);
 int pci_epf_bind(struct pci_epf *epf);
-- 
2.43.0

Re: [PATCH 1/3] PCI: endpoint: refactor pci_epf_alloc_space()

[Frank Li]

On Tue, Jan 30, 2024 at 08:32:09PM +0100, Niklas Cassel wrote:
> Refactor pci_epf_alloc_space() to take epc_features as a parameter.
> This is a preparation patch needed for further cleanups.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/endpoint/functions/pci-epf-ntb.c  | 2 +-
>  drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 4 ++--
>  drivers/pci/endpoint/pci-epf-core.c           | 6 ++++--
>  include/linux/pci-epf.h                       | 4 +++-
>  5 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> index 0553946005c4..43cd309ce22f 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> @@ -1067,7 +1067,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb,
>  	else if (size < ctrl_size + spad_size)
>  		return -EINVAL;
>  
> -	base = pci_epf_alloc_space(epf, size, barno, align, type);
> +	base = pci_epf_alloc_space(epf, size, barno, epc_features, type);
>  	if (!base) {
>  		dev_err(dev, "%s intf: Config/Status/SPAD alloc region fail\n",
>  			pci_epc_interface_string(type));
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 18c80002d3bd..15bfa7d83489 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -848,7 +848,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	}
>  
>  	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
> -				   epc_features->align, PRIMARY_INTERFACE);
> +				   epc_features, PRIMARY_INTERFACE);
>  	if (!base) {
>  		dev_err(dev, "Failed to allocated register space\n");
>  		return -ENOMEM;
> @@ -866,8 +866,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  			continue;
>  
>  		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> -					   epc_features->align,
> -					   PRIMARY_INTERFACE);
> +					   epc_features, PRIMARY_INTERFACE);
>  		if (!base)
>  			dev_err(dev, "Failed to allocate space for BAR%d\n",
>  				bar);
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index e75a2af77328..ba509d67188b 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -446,7 +446,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  	else if (size < ctrl_size + spad_size)
>  		return -EINVAL;
>  
> -	base = pci_epf_alloc_space(epf, size, barno, align, 0);
> +	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
>  	if (!base) {
>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
>  		return -ENOMEM;
> @@ -550,7 +550,7 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  
>  	barno = ntb->epf_ntb_bar[BAR_DB];
>  
> -	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> +	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
>  	if (!mw_addr) {
>  		dev_err(dev, "Failed to allocate OB address\n");
>  		return -ENOMEM;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 2c32de667937..e44f4078fe8b 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -251,14 +251,16 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
>   * @epf: the EPF device to whom allocate the memory
>   * @size: the size of the memory that has to be allocated
>   * @bar: the BAR number corresponding to the allocated register space
> - * @align: alignment size for the allocation region
> + * @epc: the features provided by the EPC specific to this endpoint function
>   * @type: Identifies if the allocation is for primary EPC or secondary EPC
>   *
>   * Invoke to allocate memory for the PCI EPF register space.
>   */
>  void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> -			  size_t align, enum pci_epc_interface_type type)
> +			  const struct pci_epc_features *epc_features,
> +			  enum pci_epc_interface_type type)
>  {
> +	size_t align = epc_features->align;
>  	struct pci_epf_bar *epf_bar;
>  	dma_addr_t phys_addr;
>  	struct pci_epc *epc;
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 77b146e0f672..adee6a1b35db 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -15,6 +15,7 @@
>  #include <linux/pci.h>
>  
>  struct pci_epf;
> +struct pci_epc_features;
>  enum pci_epc_interface_type;
>  
>  enum pci_barno {
> @@ -216,7 +217,8 @@ int __pci_epf_register_driver(struct pci_epf_driver *driver,
>  			      struct module *owner);
>  void pci_epf_unregister_driver(struct pci_epf_driver *driver);
>  void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> -			  size_t align, enum pci_epc_interface_type type);
> +			  const struct pci_epc_features *epc_features,
> +			  enum pci_epc_interface_type type);
>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>  			enum pci_epc_interface_type type);
>  int pci_epf_bind(struct pci_epf *epf);
> -- 
> 2.43.0
> 

Re: [PATCH 1/3] PCI: endpoint: refactor pci_epf_alloc_space()

[Manivannan Sadhasivam]

On Tue, Jan 30, 2024 at 08:32:09PM +0100, Niklas Cassel wrote:
> Refactor pci_epf_alloc_space() to take epc_features as a parameter.
> This is a preparation patch needed for further cleanups.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

One comment below. With that addressed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/pci/endpoint/functions/pci-epf-ntb.c  | 2 +-
>  drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 4 ++--
>  drivers/pci/endpoint/pci-epf-core.c           | 6 ++++--
>  include/linux/pci-epf.h                       | 4 +++-
>  5 files changed, 12 insertions(+), 9 deletions(-)
> 

[...]

> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 2c32de667937..e44f4078fe8b 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -251,14 +251,16 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
>   * @epf: the EPF device to whom allocate the memory
>   * @size: the size of the memory that has to be allocated
>   * @bar: the BAR number corresponding to the allocated register space
> - * @align: alignment size for the allocation region
> + * @epc: the features provided by the EPC specific to this endpoint function
>   * @type: Identifies if the allocation is for primary EPC or secondary EPC
>   *
>   * Invoke to allocate memory for the PCI EPF register space.
>   */
>  void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> -			  size_t align, enum pci_epc_interface_type type)
> +			  const struct pci_epc_features *epc_features,

s/epc/epc_features

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 2/3] PCI: endpoint: improve pci_epf_alloc_space()

[Niklas Cassel]

pci_epf_alloc_space() already performs checks on the requested BAR size,
and will allocate and set epf_bar->size to a size higher than the
requested BAR size if some constraint deems it necessary.

However, other than pci_epf_alloc_space() performing these roundups,
there are checks and roundups in two different places in pci-epf-test.c.

And further checks are proposed to other endpoint function drivers, see:
https://lore.kernel.org/linux-pci/20240108151015.2030469-1-Frank.Li@nxp.com/

Having these checks spread out at different places in the EPF driver
(and potentially in multiple EPF drivers) is not maintainable and makes
the code hard to follow.

Since pci_epf_alloc_space() already performs roundups, move the checks and
roundups performed by pci-epf-test.c to pci_epf_alloc_space().

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c |  8 --------
 drivers/pci/endpoint/pci-epf-core.c           | 10 +++++++++-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 15bfa7d83489..981894e40681 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -841,12 +841,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	}
 	test_reg_size = test_reg_bar_size + msix_table_size + pba_size;
 
-	if (epc_features->bar_fixed_size[test_reg_bar]) {
-		if (test_reg_size > bar_size[test_reg_bar])
-			return -ENOMEM;
-		test_reg_size = bar_size[test_reg_bar];
-	}
-
 	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
 				   epc_features, PRIMARY_INTERFACE);
 	if (!base) {
@@ -888,8 +882,6 @@ static void pci_epf_configure_bar(struct pci_epf *epf,
 		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
 		if (bar_fixed_64bit)
 			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-		if (epc_features->bar_fixed_size[i])
-			bar_size[i] = epc_features->bar_fixed_size[i];
 	}
 }
 
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index e44f4078fe8b..37d9651d2026 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -260,6 +260,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 			  const struct pci_epc_features *epc_features,
 			  enum pci_epc_interface_type type)
 {
+	u64 bar_fixed_size = epc_features->bar_fixed_size[bar];
 	size_t align = epc_features->align;
 	struct pci_epf_bar *epf_bar;
 	dma_addr_t phys_addr;
@@ -270,7 +271,14 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 	if (size < 128)
 		size = 128;
 
-	if (align)
+	if (bar_fixed_size && size > bar_fixed_size) {
+		dev_err(dev, "requested BAR size is larger than fixed size\n");
+		return NULL;
+	}
+
+	if (bar_fixed_size)
+		size = bar_fixed_size;
+	else if (align)
 		size = ALIGN(size, align);
 	else
 		size = roundup_pow_of_two(size);
-- 
2.43.0

Re: [PATCH 2/3] PCI: endpoint: improve pci_epf_alloc_space()

[Frank Li]

On Tue, Jan 30, 2024 at 08:32:10PM +0100, Niklas Cassel wrote:
> pci_epf_alloc_space() already performs checks on the requested BAR size,
> and will allocate and set epf_bar->size to a size higher than the
> requested BAR size if some constraint deems it necessary.
> 
> However, other than pci_epf_alloc_space() performing these roundups,
> there are checks and roundups in two different places in pci-epf-test.c.
> 
> And further checks are proposed to other endpoint function drivers, see:
> https://lore.kernel.org/linux-pci/20240108151015.2030469-1-Frank.Li@nxp.com/
> 
> Having these checks spread out at different places in the EPF driver
> (and potentially in multiple EPF drivers) is not maintainable and makes
> the code hard to follow.
> 
> Since pci_epf_alloc_space() already performs roundups, move the checks and
> roundups performed by pci-epf-test.c to pci_epf_alloc_space().
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c |  8 --------
>  drivers/pci/endpoint/pci-epf-core.c           | 10 +++++++++-
>  2 files changed, 9 insertions(+), 9 deletions(-)

Suggest split to two patches. One change epf-core, the other one clean
up epf-test, or you can combine epf-vntb to it, which clean up checking
code.

Frank
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 15bfa7d83489..981894e40681 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -841,12 +841,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	}
>  	test_reg_size = test_reg_bar_size + msix_table_size + pba_size;
>  
> -	if (epc_features->bar_fixed_size[test_reg_bar]) {
> -		if (test_reg_size > bar_size[test_reg_bar])
> -			return -ENOMEM;
> -		test_reg_size = bar_size[test_reg_bar];
> -	}
> -
>  	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
>  				   epc_features, PRIMARY_INTERFACE);
>  	if (!base) {
> @@ -888,8 +882,6 @@ static void pci_epf_configure_bar(struct pci_epf *epf,
>  		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
>  		if (bar_fixed_64bit)
>  			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -		if (epc_features->bar_fixed_size[i])
> -			bar_size[i] = epc_features->bar_fixed_size[i];
>  	}
>  }
>  
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index e44f4078fe8b..37d9651d2026 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -260,6 +260,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			  const struct pci_epc_features *epc_features,
>  			  enum pci_epc_interface_type type)
>  {
> +	u64 bar_fixed_size = epc_features->bar_fixed_size[bar];
>  	size_t align = epc_features->align;
>  	struct pci_epf_bar *epf_bar;
>  	dma_addr_t phys_addr;
> @@ -270,7 +271,14 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	if (size < 128)
>  		size = 128;
>  
> -	if (align)
> +	if (bar_fixed_size && size > bar_fixed_size) {
> +		dev_err(dev, "requested BAR size is larger than fixed size\n");
> +		return NULL;
> +	}
> +
> +	if (bar_fixed_size)
> +		size = bar_fixed_size;
> +	else if (align)
>  		size = ALIGN(size, align);
>  	else
>  		size = roundup_pow_of_two(size);
> -- 
> 2.43.0
> 

Re: [PATCH 2/3] PCI: endpoint: improve pci_epf_alloc_space()

[Niklas Cassel]

On Tue, Jan 30, 2024 at 08:32:10PM +0100, Niklas Cassel wrote:
> pci_epf_alloc_space() already performs checks on the requested BAR size,
> and will allocate and set epf_bar->size to a size higher than the
> requested BAR size if some constraint deems it necessary.
> 
> However, other than pci_epf_alloc_space() performing these roundups,
> there are checks and roundups in two different places in pci-epf-test.c.
> 
> And further checks are proposed to other endpoint function drivers, see:
> https://lore.kernel.org/linux-pci/20240108151015.2030469-1-Frank.Li@nxp.com/
> 
> Having these checks spread out at different places in the EPF driver
> (and potentially in multiple EPF drivers) is not maintainable and makes
> the code hard to follow.
> 
> Since pci_epf_alloc_space() already performs roundups, move the checks and
> roundups performed by pci-epf-test.c to pci_epf_alloc_space().
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c |  8 --------
>  drivers/pci/endpoint/pci-epf-core.c           | 10 +++++++++-
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 15bfa7d83489..981894e40681 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -841,12 +841,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	}
>  	test_reg_size = test_reg_bar_size + msix_table_size + pba_size;
>  
> -	if (epc_features->bar_fixed_size[test_reg_bar]) {
> -		if (test_reg_size > bar_size[test_reg_bar])
> -			return -ENOMEM;
> -		test_reg_size = bar_size[test_reg_bar];
> -	}
> -
>  	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
>  				   epc_features, PRIMARY_INTERFACE);
>  	if (!base) {
> @@ -888,8 +882,6 @@ static void pci_epf_configure_bar(struct pci_epf *epf,
>  		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
>  		if (bar_fixed_64bit)
>  			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -		if (epc_features->bar_fixed_size[i])
> -			bar_size[i] = epc_features->bar_fixed_size[i];
>  	}
>  }
>  
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index e44f4078fe8b..37d9651d2026 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -260,6 +260,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			  const struct pci_epc_features *epc_features,
>  			  enum pci_epc_interface_type type)
>  {
> +	u64 bar_fixed_size = epc_features->bar_fixed_size[bar];
>  	size_t align = epc_features->align;
>  	struct pci_epf_bar *epf_bar;
>  	dma_addr_t phys_addr;
> @@ -270,7 +271,14 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	if (size < 128)
>  		size = 128;
>  
> -	if (align)
> +	if (bar_fixed_size && size > bar_fixed_size) {
> +		dev_err(dev, "requested BAR size is larger than fixed size\n");
> +		return NULL;
> +	}
> +
> +	if (bar_fixed_size)
> +		size = bar_fixed_size;
> +	else if (align)
>  		size = ALIGN(size, align);
>  	else
>  		size = roundup_pow_of_two(size);

We actually need to perform the alignment even for fixed size BARs too,
since some platforms have fixed_size_bars that are smaller than the
iATU MIN REGION.

Will fix in v2.


Kind regards,
Niklas

Re: [PATCH 2/3] PCI: endpoint: improve pci_epf_alloc_space()

[Manivannan Sadhasivam]

On Tue, Jan 30, 2024 at 08:32:10PM +0100, Niklas Cassel wrote:
> pci_epf_alloc_space() already performs checks on the requested BAR size,
> and will allocate and set epf_bar->size to a size higher than the
> requested BAR size if some constraint deems it necessary.
> 
> However, other than pci_epf_alloc_space() performing these roundups,
> there are checks and roundups in two different places in pci-epf-test.c.
> 
> And further checks are proposed to other endpoint function drivers, see:
> https://lore.kernel.org/linux-pci/20240108151015.2030469-1-Frank.Li@nxp.com/
> 
> Having these checks spread out at different places in the EPF driver
> (and potentially in multiple EPF drivers) is not maintainable and makes
> the code hard to follow.
> 
> Since pci_epf_alloc_space() already performs roundups, move the checks and
> roundups performed by pci-epf-test.c to pci_epf_alloc_space().
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Please move the epf-test change to patch 2. Rest LGTM.

- Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c |  8 --------
>  drivers/pci/endpoint/pci-epf-core.c           | 10 +++++++++-
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 15bfa7d83489..981894e40681 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -841,12 +841,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	}
>  	test_reg_size = test_reg_bar_size + msix_table_size + pba_size;
>  
> -	if (epc_features->bar_fixed_size[test_reg_bar]) {
> -		if (test_reg_size > bar_size[test_reg_bar])
> -			return -ENOMEM;
> -		test_reg_size = bar_size[test_reg_bar];
> -	}
> -
>  	base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
>  				   epc_features, PRIMARY_INTERFACE);
>  	if (!base) {
> @@ -888,8 +882,6 @@ static void pci_epf_configure_bar(struct pci_epf *epf,
>  		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
>  		if (bar_fixed_64bit)
>  			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -		if (epc_features->bar_fixed_size[i])
> -			bar_size[i] = epc_features->bar_fixed_size[i];
>  	}
>  }
>  
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index e44f4078fe8b..37d9651d2026 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -260,6 +260,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			  const struct pci_epc_features *epc_features,
>  			  enum pci_epc_interface_type type)
>  {
> +	u64 bar_fixed_size = epc_features->bar_fixed_size[bar];
>  	size_t align = epc_features->align;
>  	struct pci_epf_bar *epf_bar;
>  	dma_addr_t phys_addr;
> @@ -270,7 +271,14 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	if (size < 128)
>  		size = 128;
>  
> -	if (align)
> +	if (bar_fixed_size && size > bar_fixed_size) {
> +		dev_err(dev, "requested BAR size is larger than fixed size\n");
> +		return NULL;
> +	}
> +
> +	if (bar_fixed_size)
> +		size = bar_fixed_size;
> +	else if (align)
>  		size = ALIGN(size, align);
>  	else
>  		size = roundup_pow_of_two(size);
> -- 
> 2.43.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 3/3] PCI: endpoint: pci-epf-vntb: remove superfluous checks

[Niklas Cassel]

Remove superfluous alignment checks, these checks are already done by
pci_epf_alloc_space().

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index ba509d67188b..eda4b906868b 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -527,7 +527,6 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 {
 	const struct pci_epc_features *epc_features;
-	u32 align;
 	struct device *dev = &ntb->epf->dev;
 	int ret;
 	struct pci_epf_bar *epf_bar;
@@ -538,16 +537,6 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	epc_features = pci_epc_get_features(ntb->epf->epc,
 					    ntb->epf->func_no,
 					    ntb->epf->vfunc_no);
-	align = epc_features->align;
-
-	if (size < 128)
-		size = 128;
-
-	if (align)
-		size = ALIGN(size, align);
-	else
-		size = roundup_pow_of_two(size);
-
 	barno = ntb->epf_ntb_bar[BAR_DB];
 
 	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
-- 
2.43.0

Re: [PATCH 3/3] PCI: endpoint: pci-epf-vntb: remove superfluous checks

[Frank Li]

On Tue, Jan 30, 2024 at 08:32:11PM +0100, Niklas Cassel wrote:
> Remove superfluous alignment checks, these checks are already done by
> pci_epf_alloc_space().
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index ba509d67188b..eda4b906868b 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -527,7 +527,6 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
>  static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  {
>  	const struct pci_epc_features *epc_features;
> -	u32 align;
>  	struct device *dev = &ntb->epf->dev;
>  	int ret;
>  	struct pci_epf_bar *epf_bar;
> @@ -538,16 +537,6 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  	epc_features = pci_epc_get_features(ntb->epf->epc,
>  					    ntb->epf->func_no,
>  					    ntb->epf->vfunc_no);
> -	align = epc_features->align;
> -
> -	if (size < 128)
> -		size = 128;
> -
> -	if (align)
> -		size = ALIGN(size, align);
> -	else
> -		size = roundup_pow_of_two(size);
> -
>  	barno = ntb->epf_ntb_bar[BAR_DB];
>  
>  	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
> -- 
> 2.43.0
>