Re: [PATCH 07/12] wifi: mwifiex: fix array of flexible structures warnings

From: Johannes Berg <johannes@sipsolutions.net>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [PATCH 07/12] wifi: mwifiex: fix array of flexible structures warnings
Date: Wed, 07 Sep 2022 08:57:28 +0200	[thread overview]
Message-ID: <b6f5ad5b9a50fd26838c019921f3ace1e739f9b8.camel@sipsolutions.net> (raw)
In-Reply-To: <YxfHwxuwEP1rQoAU@google.com>

On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote:
> Hi,
> 
> On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > There are two, just change them to have a "u8 data[]" type
> > member, and add casts where needed. No binary changes.
> 
> Hmm, what exact warning are you looking at? This one?
> https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
> 

I think gcc also has one with W=1 now?

But yes, it's similar to that one. I was looking at Jakub's netdev test
builds here:

https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr

../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures

> > @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> >  struct host_cmd_ds_mef_cfg {
> >  	__le32 criteria;
> >  	__le16 num_entries;
> > -	struct mwifiex_fw_mef_entry mef_entry[];
> > +	u8 mef_entry_data[];
> 
> Do you actually need this part of the change? The 'mef_entry' (or
> 'mef_entry_data') field is not actually used anywhere now, but I can't
> tell what kind of warning is involved.

But it itself is variable, so the compiler is (rightfully, IMHO) saying
that you can't have an array of something that has a variable size, even
if it's a variable array.

> >  struct host_cmd_ds_coalesce_cfg {
> >  	__le16 action;
> >  	__le16 num_of_rules;
> > -	struct coalesce_receive_filt_rule rule[];
> > +	u8 rule_data[];
> 
> These kinds of changes seem to be losing some valuable information. At a
> minimum, it would be nice to leave a comment that points at the intended
> struct; but ideally, we'd be able to still get the type safety from
> actually using the struct, instead of relying on casts and/or u8/void.

All fair points. This was the way we typically do this in e.g.
ieee80211.h ("u8 variable[]", not "struct element elems[]"), but YMMV.

I thought later after Kalle asked about making it a single entry such as

  struct coalesce_receive_filt_rule first_rule;

but then the sizeof() is messed up and then the change has to be much
more careful.

Anyway, I was mostly trying to remove some warnings in drivers that
don't really have a very active maintainer anymore, since we have so
many in wireless it sometimes makes it hard to see which ones are new.

No particular feelings about this patch :-)

johannes