Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b6f5ad5b9a50fd26838c019921f3ace1e739f9b8.camel@sipsolutions.net>
Subject: 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
Date:   Wed, 07 Sep 2022 08:57:28 +0200
In-Reply-To: <YxfHwxuwEP1rQoAU@google.com>
References: <20220904212910.8169e8c9090c.I0357e80cc86be2d4ac6205d1f53568444dcf7c9b@changeid>
         <20220904212910.2c885310ebee.If7177ea588b56c405eee6e6df595e9efccdfb99a@changeid>
         <YxfHwxuwEP1rQoAU@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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

I think gcc also has one with W=3D1 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 fle=
xible structures
../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of fle=
xible 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[];
>=20
> 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[];
>=20
> 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