On Wed, Sep 07, 2022 at 08:57:28AM +0200, Johannes Berg wrote: > On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote: > > On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote: > > > From: Johannes Berg <johannes.berg@xxxxxxxxx> > > > > > > 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 I think you're actually looking at a sparse warning: https://lore.kernel.org/linux-sparse/20200930231828.66751-12-luc.vanoostenryck@xxxxxxxxx/ I can convince clang to enable the warning I mentioned, but it's not on by default, even with W=1. When enabled, it complains similarly, as well as about a ton of other code too. > > > @@ -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. OK. I suppose this warning makes sense when you're truly treating these as arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg() and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly handle the flexible array is pretty ugly anyway, and doesn't really use the type safety much (lots of casting through u8* and similar). So this isn't really the best example to try to retain. (mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.) But if the "array" is just an optional extension to a struct, and we expect at most a single element, then I don't think the construct is fundamentally wrong. It might still be hard to get correct in all cases (e.g., ensuring the right buffer size), but it still feels like a decent way to describe things. > > > 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. Sure, I guess it's good to clean some of these up. Looking around, I don't see a great alternative, and per some of my above notes, I don't think these are beautiful as-is. > No particular feelings about this patch :-) After a little more look, I'm fine with this patch. I'd probably appreciate it better if there was a comment of some kind in the struct definition, and perhaps mention sparse's -Wflexible-array-array. But either way: Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx> Thanks.
