Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

[Jiri Slaby <jirislaby@kernel.org>]

On 02. 06. 23, 6:36, Shiji Yang wrote:
> From: Shiji Yang <yangshiji66@qq.com>
> 
> Add a struct_group to around all members in struct ath_cycle_counters.
> It can help the compiler detect the intended bounds of the memcpy() and
> memset().
> 
> This patch fixes the following build warning:
> 
> In function 'fortify_memset_chk',
>      inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>    314 |                         __write_overflow_field(p_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 

Hi,

what compiler/version is this with?

> Signed-off-by: Shiji Yang <yangshiji66@qq.com>
> ---
> More discussion on: https://github.com/openwrt/openwrt/pull/12764

No "__write_overflow_field" there. Is this the right link?

> ---
>   drivers/net/wireless/ath/ath.h                | 10 ++++++----
>   drivers/net/wireless/ath/ath5k/ani.c          |  2 +-
>   drivers/net/wireless/ath/ath5k/base.c         |  4 ++--
>   drivers/net/wireless/ath/ath5k/mac80211-ops.c |  2 +-
>   drivers/net/wireless/ath/ath9k/link.c         |  2 +-
>   drivers/net/wireless/ath/ath9k/main.c         |  4 ++--
>   drivers/net/wireless/ath/hw.c                 |  2 +-
>   7 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
> index f02a308a9..4b42e401a 100644
> --- a/drivers/net/wireless/ath/ath.h
> +++ b/drivers/net/wireless/ath/ath.h
> @@ -43,10 +43,12 @@ struct ath_ani {
>   };
>   
>   struct ath_cycle_counters {
> -	u32 cycles;
> -	u32 rx_busy;
> -	u32 rx_frame;
> -	u32 tx_frame;
> +	struct_group(cnts,
> +		u32 cycles;
> +		u32 rx_busy;
> +		u32 rx_frame;
> +		u32 tx_frame;
> +	);

This is horrid.

>   };
>   
>   enum ath_device_state {
> diff --git a/drivers/net/wireless/ath/ath5k/ani.c b/drivers/net/wireless/ath/ath5k/ani.c
> index 850c608b4..fa95f0f0f 100644
> --- a/drivers/net/wireless/ath/ath5k/ani.c
> +++ b/drivers/net/wireless/ath/ath5k/ani.c
> @@ -379,7 +379,7 @@ ath5k_hw_ani_get_listen_time(struct ath5k_hw *ah, struct ath5k_ani_state *as)
>   	spin_lock_bh(&common->cc_lock);
>   
>   	ath_hw_cycle_counters_update(common);
> -	memcpy(&as->last_cc, &common->cc_ani, sizeof(as->last_cc));
> +	memcpy(&as->last_cc.cnts, &common->cc_ani.cnts, sizeof(as->last_cc.cnts));

So is this.

Care to elaborate why this is needed at all, provided we copy/zero a 
whole structure? And describe it in the commit log, not in random 
external sources.

thanks,
-- 
js
suse labs