[PATCH] brcmfmac: fwsignal: Use struct_size() to simplify brcmf_fws_rxreorder()

[PATCH] brcmfmac: fwsignal: Use struct_size() to simplify brcmf_fws_rxreorder()

[Christophe JAILLET]

In the "struct brcmf_ampdu_rx_reorder", change the 'pktslots' field into
flexible array.

It saves the size of a pointer when the memory is allocated and avoids
an indirection when the array is used.
It also removes the usage of a pointer arithmetic and saves a few lines of
code.

Finally, struct_size() can be used. It is not a must have here, because
it is easy to see that buf_size can not overflow, but still, it is a good
practice.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h   | 4 ++--
 .../net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c   | 8 ++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index ea76b8d33401..6ea2b677f047 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -48,20 +48,20 @@
 /**
  * struct brcmf_ampdu_rx_reorder - AMPDU receive reorder info
  *
- * @pktslots: dynamic allocated array for ordering AMPDU packets.
  * @flow_id: AMPDU flow identifier.
  * @cur_idx: last AMPDU index from firmware.
  * @exp_idx: expected next AMPDU index.
  * @max_idx: maximum amount of packets per AMPDU.
  * @pend_pkts: number of packets currently in @pktslots.
+ * @pktslots: dynamic allocated array for ordering AMPDU packets.
  */
 struct brcmf_ampdu_rx_reorder {
-	struct sk_buff **pktslots;
 	u8 flow_id;
 	u8 cur_idx;
 	u8 exp_idx;
 	u8 max_idx;
 	u8 pend_pkts;
+	struct sk_buff *pktslots[];
 };
 
 /* Forward decls for struct brcmf_pub (see below) */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index 36af81975855..0949e7975ff1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -1673,7 +1673,6 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
 	struct sk_buff_head reorder_list;
 	struct sk_buff *pnext;
 	u8 flags;
-	u32 buf_size;
 
 	reorder_data = ((struct brcmf_skb_reorder_data *)pkt->cb)->reorder;
 	flow_id = reorder_data[BRCMF_RXREORDER_FLOWID_OFFSET];
@@ -1708,15 +1707,13 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
 	}
 	/* from here on we need a flow reorder instance */
 	if (rfi == NULL) {
-		buf_size = sizeof(*rfi);
 		max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET];
 
-		buf_size += (max_idx + 1) * sizeof(pkt);
-
 		/* allocate space for flow reorder info */
 		brcmf_dbg(INFO, "flow-%d: start, maxidx %d\n",
 			  flow_id, max_idx);
-		rfi = kzalloc(buf_size, GFP_ATOMIC);
+		rfi = kzalloc(struct_size(rfi, pktslots, max_idx + 1),
+			      GFP_ATOMIC);
 		if (rfi == NULL) {
 			bphy_err(drvr, "failed to alloc buffer\n");
 			brcmf_netif_rx(ifp, pkt);
@@ -1724,7 +1721,6 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt)
 		}
 
 		ifp->drvr->reorder_flows[flow_id] = rfi;
-		rfi->pktslots = (struct sk_buff **)(rfi + 1);
 		rfi->max_idx = max_idx;
 	}
 	if (flags & BRCMF_RXREORDER_NEW_HOLE)  {
-- 
2.45.2

Re: [PATCH] brcmfmac: fwsignal: Use struct_size() to simplify brcmf_fws_rxreorder()

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On 7/14/2024 12:10 PM, Christophe JAILLET wrote:
> In the "struct brcmf_ampdu_rx_reorder", change the 'pktslots' field into
> flexible array.
> 
> It saves the size of a pointer when the memory is allocated and avoids
> an indirection when the array is used.
> It also removes the usage of a pointer arithmetic and saves a few lines of
> code.
> 
> Finally, struct_size() can be used. It is not a must have here, because
> it is easy to see that buf_size can not overflow, but still, it is a good
> practice.

Looks good to me with a minor nit (see below)...

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h   | 4 ++--
>   .../net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c   | 8 ++------
>   2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index ea76b8d33401..6ea2b677f047 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -48,20 +48,20 @@
>   /**
>    * struct brcmf_ampdu_rx_reorder - AMPDU receive reorder info
>    *
> - * @pktslots: dynamic allocated array for ordering AMPDU packets.
>    * @flow_id: AMPDU flow identifier.
>    * @cur_idx: last AMPDU index from firmware.
>    * @exp_idx: expected next AMPDU index.
>    * @max_idx: maximum amount of packets per AMPDU.
>    * @pend_pkts: number of packets currently in @pktslots.
> + * @pktslots: dynamic allocated array for ordering AMPDU packets.

pktslots is not dynamic allocated, but just part of the structure. That 
was already so before this patch through. Just drop the "dynamic 
allocated" part.

>    */
>   struct brcmf_ampdu_rx_reorder {
> -	struct sk_buff **pktslots;
>   	u8 flow_id;
>   	u8 cur_idx;
>   	u8 exp_idx;
>   	u8 max_idx;
>   	u8 pend_pkts;
> +	struct sk_buff *pktslots[];
>   };
>   
>   /* Forward decls for struct brcmf_pub (see below) */

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]