| Message ID | 20231213-wmi_host_mem_chunks_flexarray-v1-6-92922d92fa2c@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6b9923f1f6d1b57a2ae932540a6273a0face54fe |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath10k: use flexible arrays | expand |
On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote: > In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update() > the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function > ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it. Is .gen_tdls_peer_update only ever called after a fresh allocation? It wasn't obvious to me as I tried to follow the call paths. Is there harm in leaving this? -Kees > > No functional changes, compile tested only. > > [1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/ > > Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> > --- > drivers/net/wireless/ath/ath10k/wmi.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 4d5aadbc7159..0cfd9484c45e 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar, > if (!skb) > return ERR_PTR(-ENOMEM); > > - memset(skb->data, 0, sizeof(*cmd)); > - > cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data; > cmd->vdev_id = __cpu_to_le32(arg->vdev_id); > ether_addr_copy(cmd->peer_macaddr.addr, arg->addr); > > -- > 2.42.0 >
On 12/13/2023 11:16 AM, Kees Cook wrote: > On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote: >> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update() >> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function >> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it. > > Is .gen_tdls_peer_update only ever called after a fresh allocation? It > wasn't obvious to me as I tried to follow the call paths. Is there harm > in leaving this? The only harm is a slight increase in code size and cpu cycles. However note the skb allocation is done within ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code being removed: skb = ath10k_wmi_alloc_skb(ar, len); if (!skb) return ERR_PTR(-ENOMEM); And in ath10k_wmi_alloc_skb() we have: memset(skb->data, 0, round_len); So the memset() being removed is always redundant. /jeff
On Wed, Dec 13, 2023 at 11:36:08AM -0800, Jeff Johnson wrote: > On 12/13/2023 11:16 AM, Kees Cook wrote: > > On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote: > >> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update() > >> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function > >> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it. > > > > Is .gen_tdls_peer_update only ever called after a fresh allocation? It > > wasn't obvious to me as I tried to follow the call paths. Is there harm > > in leaving this? > > The only harm is a slight increase in code size and cpu cycles. > > However note the skb allocation is done within > ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code > being removed: > skb = ath10k_wmi_alloc_skb(ar, len); > if (!skb) > return ERR_PTR(-ENOMEM); > > And in ath10k_wmi_alloc_skb() we have: > memset(skb->data, 0, round_len); > > So the memset() being removed is always redundant. LOL. I see now. I missed that was was looking outside the function! :P Reviewed-by: Kees Cook <keescook@chromium.org>
On 12/13/23 11:06, Jeff Johnson wrote: > In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update() > the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function > ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it. > > No functional changes, compile tested only. > > [1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/ > > Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks! -- Gustavo > --- > drivers/net/wireless/ath/ath10k/wmi.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 4d5aadbc7159..0cfd9484c45e 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar, > if (!skb) > return ERR_PTR(-ENOMEM); > > - memset(skb->data, 0, sizeof(*cmd)); > - > cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data; > cmd->vdev_id = __cpu_to_le32(arg->vdev_id); > ether_addr_copy(cmd->peer_macaddr.addr, arg->addr); >
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 4d5aadbc7159..0cfd9484c45e 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar, if (!skb) return ERR_PTR(-ENOMEM); - memset(skb->data, 0, sizeof(*cmd)); - cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data; cmd->vdev_id = __cpu_to_le32(arg->vdev_id); ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);
In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update() the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it. No functional changes, compile tested only. [1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/ Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> --- drivers/net/wireless/ath/ath10k/wmi.c | 2 -- 1 file changed, 2 deletions(-)