[PATCH] soc: qcom: ipa: kill IPA_RX_BUFFER_ORDER

[PATCH] soc: qcom: ipa: kill IPA_RX_BUFFER_ORDER

[Alex Elder]

Don't assume the receive buffer size is a power-of-2 number of pages.
Instead, define the receive buffer size independently, and then
compute the page order from that size when needed.

This fixes a build problem that arises when the ARM64_PAGE_SHIFT
config option is set to have a page size greater than 4KB.  The
problem was identified by Linux Kernel Functional Testing.

The IPA code basically assumed the page size to be 4KB.  A larger page
size caused the receive buffer size to become correspondingly larger
(32KB or 128KB for ARM64_16K_PAGES and ARM64_64K_PAGES, respectively).
The receive buffer size is used to compute an "aggregation byte limit"
value that gets programmed into the hardware, and the large page sizes
caused that limit value to be too big to fit in a 5 bit field.  This
triggered a BUILD_BUG_ON() call in ipa_endpoint_validate_build().

This fix causes a lot of receive buffer memory to be wasted if
system is configured for page size greater than 4KB.  But such a
misguided configuration will now build successfully.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Alex Elder <elder@linaro.org>
---

Dave, I *hope* this is it for IPA for this release.	-Alex

 drivers/net/ipa/ipa_endpoint.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 217cbf337ad7..6de03be28784 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -26,8 +26,8 @@
 
 #define IPA_REPLENISH_BATCH	16
 
-#define IPA_RX_BUFFER_SIZE	(PAGE_SIZE << IPA_RX_BUFFER_ORDER)
-#define IPA_RX_BUFFER_ORDER	1	/* 8KB endpoint RX buffers (2 pages) */
+/* RX buffer is 1 page (or a power-of-2 contiguous pages) */
+#define IPA_RX_BUFFER_SIZE	8192	/* PAGE_SIZE > 4096 wastes a LOT */
 
 /* The amount of RX buffer space consumed by standard skb overhead */
 #define IPA_RX_BUFFER_OVERHEAD	(PAGE_SIZE - SKB_MAX_ORDER(NET_SKB_PAD, 0))
@@ -758,7 +758,7 @@ static int ipa_endpoint_replenish_one(struct ipa_endpoint *endpoint)
 	u32 len;
 	int ret;
 
-	page = dev_alloc_pages(IPA_RX_BUFFER_ORDER);
+	page = dev_alloc_pages(get_order(IPA_RX_BUFFER_SIZE));
 	if (!page)
 		return -ENOMEM;
 
@@ -787,7 +787,7 @@ static int ipa_endpoint_replenish_one(struct ipa_endpoint *endpoint)
 err_trans_free:
 	gsi_trans_free(trans);
 err_free_pages:
-	__free_pages(page, IPA_RX_BUFFER_ORDER);
+	__free_pages(page, get_order(IPA_RX_BUFFER_SIZE));
 
 	return -ENOMEM;
 }
@@ -1073,7 +1073,7 @@ void ipa_endpoint_trans_release(struct ipa_endpoint *endpoint,
 		struct page *page = trans->data;
 
 		if (page)
-			__free_pages(page, IPA_RX_BUFFER_ORDER);
+			__free_pages(page, get_order(IPA_RX_BUFFER_SIZE));
 	}
 }
 
-- 
2.20.1

Re: [PATCH] soc: qcom: ipa: kill IPA_RX_BUFFER_ORDER

[Bjorn Andersson]

On Fri 20 Mar 09:02 PDT 2020, Alex Elder wrote:

> Don't assume the receive buffer size is a power-of-2 number of pages.
> Instead, define the receive buffer size independently, and then
> compute the page order from that size when needed.
> 
> This fixes a build problem that arises when the ARM64_PAGE_SHIFT
> config option is set to have a page size greater than 4KB.  The
> problem was identified by Linux Kernel Functional Testing.
> 
> The IPA code basically assumed the page size to be 4KB.  A larger page
> size caused the receive buffer size to become correspondingly larger
> (32KB or 128KB for ARM64_16K_PAGES and ARM64_64K_PAGES, respectively).
> The receive buffer size is used to compute an "aggregation byte limit"
> value that gets programmed into the hardware, and the large page sizes
> caused that limit value to be too big to fit in a 5 bit field.  This
> triggered a BUILD_BUG_ON() call in ipa_endpoint_validate_build().
> 
> This fix causes a lot of receive buffer memory to be wasted if
> system is configured for page size greater than 4KB.  But such a
> misguided configuration will now build successfully.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Alex Elder <elder@linaro.org>

Seems better than relying on PAGE_SIZE.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Dave, I *hope* this is it for IPA for this release.	-Alex
> 
>  drivers/net/ipa/ipa_endpoint.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
> index 217cbf337ad7..6de03be28784 100644
> --- a/drivers/net/ipa/ipa_endpoint.c
> +++ b/drivers/net/ipa/ipa_endpoint.c
> @@ -26,8 +26,8 @@
>  
>  #define IPA_REPLENISH_BATCH	16
>  
> -#define IPA_RX_BUFFER_SIZE	(PAGE_SIZE << IPA_RX_BUFFER_ORDER)
> -#define IPA_RX_BUFFER_ORDER	1	/* 8KB endpoint RX buffers (2 pages) */
> +/* RX buffer is 1 page (or a power-of-2 contiguous pages) */
> +#define IPA_RX_BUFFER_SIZE	8192	/* PAGE_SIZE > 4096 wastes a LOT */
>  
>  /* The amount of RX buffer space consumed by standard skb overhead */
>  #define IPA_RX_BUFFER_OVERHEAD	(PAGE_SIZE - SKB_MAX_ORDER(NET_SKB_PAD, 0))
> @@ -758,7 +758,7 @@ static int ipa_endpoint_replenish_one(struct ipa_endpoint *endpoint)
>  	u32 len;
>  	int ret;
>  
> -	page = dev_alloc_pages(IPA_RX_BUFFER_ORDER);
> +	page = dev_alloc_pages(get_order(IPA_RX_BUFFER_SIZE));
>  	if (!page)
>  		return -ENOMEM;
>  
> @@ -787,7 +787,7 @@ static int ipa_endpoint_replenish_one(struct ipa_endpoint *endpoint)
>  err_trans_free:
>  	gsi_trans_free(trans);
>  err_free_pages:
> -	__free_pages(page, IPA_RX_BUFFER_ORDER);
> +	__free_pages(page, get_order(IPA_RX_BUFFER_SIZE));
>  
>  	return -ENOMEM;
>  }
> @@ -1073,7 +1073,7 @@ void ipa_endpoint_trans_release(struct ipa_endpoint *endpoint,
>  		struct page *page = trans->data;
>  
>  		if (page)
> -			__free_pages(page, IPA_RX_BUFFER_ORDER);
> +			__free_pages(page, get_order(IPA_RX_BUFFER_SIZE));
>  	}
>  }
>  
> -- 
> 2.20.1
> 

Re: [PATCH] soc: qcom: ipa: kill IPA_RX_BUFFER_ORDER

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Fri, 20 Mar 2020 11:02:20 -0500

> Don't assume the receive buffer size is a power-of-2 number of pages.
> Instead, define the receive buffer size independently, and then
> compute the page order from that size when needed.
> 
> This fixes a build problem that arises when the ARM64_PAGE_SHIFT
> config option is set to have a page size greater than 4KB.  The
> problem was identified by Linux Kernel Functional Testing.
> 
> The IPA code basically assumed the page size to be 4KB.  A larger page
> size caused the receive buffer size to become correspondingly larger
> (32KB or 128KB for ARM64_16K_PAGES and ARM64_64K_PAGES, respectively).
> The receive buffer size is used to compute an "aggregation byte limit"
> value that gets programmed into the hardware, and the large page sizes
> caused that limit value to be too big to fit in a 5 bit field.  This
> triggered a BUILD_BUG_ON() call in ipa_endpoint_validate_build().
> 
> This fix causes a lot of receive buffer memory to be wasted if
> system is configured for page size greater than 4KB.  But such a
> misguided configuration will now build successfully.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> 
> Dave, I *hope* this is it for IPA for this release.	-Alex

Applied to net-next, thanks.