| Message ID | bd3ad239c4d1c49b94c1ba93e48c09df98ef86cb.1720951805.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: fwsignal: Use struct_size() to simplify brcmf_fws_rxreorder() | expand |
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) */
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) {
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(-)