| Message ID | 20220904212910.2c885310ebee.If7177ea588b56c405eee6e6df595e9efccdfb99a@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4cf4cf6eb0bfceae2f61a7a724d576684f788ea6 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [01/12] wifi: ipw2100: fix warnings about non-kernel-doc | expand |
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 It's a little hard to suggest alternatives (or understand why this is the only/best way) if I don't know what the alleged bug/warning is. > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 4 ++-- > drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 26a48d8f49be..b4f945a549f7 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -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 also see the next comment: > } __packed; > > #define CONNECTION_TYPE_INFRA 0 > @@ -2254,7 +2254,7 @@ struct coalesce_receive_filt_rule { > 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. But I don't know if that's possible, as I'm not familiar with the compiler warning involved. Brian
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
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@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 I think you're actually looking at a sparse warning: https://lore.kernel.org/linux-sparse/20200930231828.66751-12-luc.vanoostenryck@gmail.com/ 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@chromium.org> Thanks.
On Fri, 2022-09-09 at 13:45 -0700, Brian Norris wrote: > > 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@gmail.com/ > > 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. Hm. I _think_ I saw it locally with just W=1, not C=1, but then that would've been with gcc, probably 12.2 from Fedora. But I didn't try again now, don't have much time. > 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. Heh, fair point. > (mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.) Yeah it kind of has to though, given that it's a variable-sized buffer with variable-sized entries... > 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. Fair enough. It's like 0 or 1, maybe, but of course there's no way to describe that to the compiler and say "I promise to never access [1] (or higher)" :) I guess the only real alternative would be to leave it as a _single_ entry, and then fix up all the sizeof() of the containing struct, if any, to be offsetof(..., first_entry). But that sort of describes a single entry should be present, which it might not be. > 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. > Sure, I guess it makes sense to also figure out more precisely where the warning appeared. I'm running off for a trip for two weeks now-ish, so not going to send it in the short term. If you wanted to do it instead, no objections, or if Kalle wants to just add a comment while applying, or whatever. In any case there are so many warnings in the drivers that still ought to _have_ a maintainer (and I'm squinting at you for ath too, Kalle) that it's pretty much irrelevant to fix this one quickly ;-) johannes
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 26a48d8f49be..b4f945a549f7 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -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[]; } __packed; #define CONNECTION_TYPE_INFRA 0 @@ -2254,7 +2254,7 @@ struct coalesce_receive_filt_rule { struct host_cmd_ds_coalesce_cfg { __le16 action; __le16 num_of_rules; - struct coalesce_receive_filt_rule rule[]; + u8 rule_data[]; } __packed; struct host_cmd_ds_multi_chan_policy { diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index 512b5bb9cf6f..e2800a831c8e 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -1435,7 +1435,7 @@ mwifiex_cmd_mef_cfg(struct mwifiex_private *priv, mef_entry = (struct mwifiex_fw_mef_entry *)pos; mef_entry->mode = mef->mef_entry[i].mode; mef_entry->action = mef->mef_entry[i].action; - pos += sizeof(*mef_cfg->mef_entry); + pos += sizeof(*mef_entry); if (mwifiex_cmd_append_rpn_expression(priv, &mef->mef_entry[i], &pos)) @@ -1631,7 +1631,7 @@ mwifiex_cmd_coalesce_cfg(struct mwifiex_private *priv, coalesce_cfg->action = cpu_to_le16(cmd_action); coalesce_cfg->num_of_rules = cpu_to_le16(cfg->num_of_rules); - rule = coalesce_cfg->rule; + rule = (void *)coalesce_cfg->rule_data; for (cnt = 0; cnt < cfg->num_of_rules; cnt++) { rule->header.type = cpu_to_le16(TLV_TYPE_COALESCE_RULE);