diff mbox series

[6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

Message ID 20231213-wmi_host_mem_chunks_flexarray-v1-6-92922d92fa2c@quicinc.com
State New
Headers show
Series wifi: ath10k: use flexible arrays | expand

Commit Message

Jeff Johnson Dec. 13, 2023, 5:06 p.m. UTC
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(-)

Comments

Kees Cook Dec. 13, 2023, 7:16 p.m. UTC | #1
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
>
Jeff Johnson Dec. 13, 2023, 7:36 p.m. UTC | #2
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
Kees Cook Dec. 13, 2023, 7:37 p.m. UTC | #3
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>
Gustavo A. R. Silva Dec. 13, 2023, 8:20 p.m. UTC | #4
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 mbox series

Patch

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);