Re: [PATCH][next] wifi: ath11k: Avoid -Wflex-array-member-not-at-end warnings

[Kalle Valo <kvalo@kernel.org>]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 8/9/2024 9:20 AM, Gustavo A. R. Silva wrote:
>
>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>> getting ready to enable it, globally.
>> 
>> Move the conflicting declaration to the end of the structure. Notice
>> that `struct ieee80211_chanctx_conf` is a flexible structure --a
>> structure that contains a flexible-array member.
>> 
>> Also, remove a couple of unused structures.
>> 
>> Fix the following warnings:
>> drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> drivers/net/wireless/ath/ath11k/dp.h:1368:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> 
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>  drivers/net/wireless/ath/ath11k/core.h |  4 +++-
>>  drivers/net/wireless/ath/ath11k/dp.h   | 23 -----------------------
>>  2 files changed, 3 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
>> index df24f0e409af..e283415dccf3 100644
>> --- a/drivers/net/wireless/ath/ath11k/core.h
>> +++ b/drivers/net/wireless/ath/ath11k/core.h
>> @@ -406,11 +406,13 @@ struct ath11k_vif {
>>  	bool wpaie_present;
>>  	bool bcca_zero_sent;
>>  	bool do_not_send_tmpl;
>> -	struct ieee80211_chanctx_conf chanctx;
>>  	struct ath11k_arp_ns_offload arp_ns_offload;
>>  	struct ath11k_rekey_data rekey_data;
>>  
>>  	struct ath11k_reg_tpc_power_info reg_tpc_info;
>> +
>> +	/* Must be last - ends in a flexible-array member. */
>> +	struct ieee80211_chanctx_conf chanctx;
>
> there is something illogical about this since the vif is allocated using
> sizeof() and hence there will never be memory allocated for the flexible
> array, and it is assigned using either struct assignment or memcpy using the
> struct size which (fortunately) would not transfer the flexible array contents:
> 		arvif->chanctx = *ctx;
>
> 		memcpy(&arvif->chanctx, ctx, sizeof(*ctx));
>
> since ath11k doesn't actually use the drv_priv[] I guess this change is OK, it
> is just strange to me.
>
> also makes me wonder why ath11k keeps a copy of the chanctx instead of just
> getting it from the underlying ieee80211_link_data. but that is outside the
> scope of this discussion.

Yeah, this doesn't look right. I don't think a driver should be copying
struct ieee80211_chanctx_conf like that. I think I'll add a comment
about this to the code:

/* FIXME: Driver should not copy struct ieee80211_chanctx_conf,
 * especially because it has a flexible array. Find a better way.
 */

Thoughts?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html