| Message ID | 20190608144947.744-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | carl9170: fix enum compare splat | expand |
Christian Lamparter <chunkeey@gmail.com> writes: > This patch fixes a noisy warning triggered by -Wenum-compare > > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ > | and ‘enum ar9170_txq’ [-Wenum-compare] > | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > | ^ > | [...] > > This is a little bit unfortunate, since the number of queues > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or > less defined by the AR9170 hardware. Is the warning enabled by default? TBH I'm not seeing how useful this warning is for kernel development. > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, > int ret; > > BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); > - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); IMHO this just makes the code worse. Does it make sense to workaround (stupid) compiler warnings like this?
On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote: > Christian Lamparter <chunkeey@gmail.com> writes: > > > This patch fixes a noisy warning triggered by -Wenum-compare > > > > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ > > | and ‘enum ar9170_txq’ [-Wenum-compare] > > | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > > | ^ > > | [...] > > > > This is a little bit unfortunate, since the number of queues > > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 > > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or > > less defined by the AR9170 hardware. > > Is the warning enabled by default? TBH I'm not seeing how useful this > warning is for kernel development. It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" in the main Makefile). I tried debian's gcc starting from 4.6 to the lastest 8.3. They all complain about it in various degrees. https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options > > --- a/drivers/net/wireless/ath/carl9170/main.c > > +++ b/drivers/net/wireless/ath/carl9170/main.c > > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, > > int ret; > > > > BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); > > - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > > + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); > > IMHO this just makes the code worse. Does it make sense to workaround > (stupid) compiler warnings like this? True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there to guard but it's getting compiled away. I could also just drop it.
Christian Lamparter <chunkeey@gmail.com> writes: > On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote: >> Christian Lamparter <chunkeey@gmail.com> writes: >> >> > This patch fixes a noisy warning triggered by -Wenum-compare >> > >> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ >> > | and ‘enum ar9170_txq’ [-Wenum-compare] >> > | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); >> > | ^ >> > | [...] >> > >> > This is a little bit unfortunate, since the number of queues >> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 >> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or >> > less defined by the AR9170 hardware. >> >> Is the warning enabled by default? TBH I'm not seeing how useful this >> warning is for kernel development. > > It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" > in the main Makefile). > > I tried debian's gcc starting from 4.6 to the lastest 8.3. They all > complain about it in various degrees. > > https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options Ok, odd that I haven't noticed this warning. Maybe I have been just blind. >> > --- a/drivers/net/wireless/ath/carl9170/main.c >> > +++ b/drivers/net/wireless/ath/carl9170/main.c >> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, >> > int ret; >> > >> > BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); >> > - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); >> > + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); >> >> IMHO this just makes the code worse. Does it make sense to workaround >> (stupid) compiler warnings like this? > > True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there > to guard but it's getting compiled away. I could also just drop it. Either way is fine for me. If the GCC (by default) emits a warning for this we need to silence that warning, so in that respect I guess we should apply this. Unless better solutions come up, of course :)
On Tuesday, June 18, 2019 2:11:53 PM CEST Kalle Valo wrote: > Christian Lamparter <chunkeey@gmail.com> writes: > > On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote: > >> Christian Lamparter <chunkeey@gmail.com> writes: > >> > >> > This patch fixes a noisy warning triggered by -Wenum-compare > >> > > >> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ > >> > | and ‘enum ar9170_txq’ [-Wenum-compare] > >> > | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > >> > | ^ > >> > | [...] > >> > > >> > This is a little bit unfortunate, since the number of queues > >> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 > >> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or > >> > less defined by the AR9170 hardware. > >> > >> Is the warning enabled by default? TBH I'm not seeing how useful this > >> warning is for kernel development. > > > > It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" > > in the main Makefile). > > > > I tried debian's gcc starting from 4.6 to the lastest 8.3. They all > > complain about it in various degrees. > > > > https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options > > Ok, odd that I haven't noticed this warning. Maybe I have been just > blind. Sorry. No, I messed up there. I made a patch back in the day during the spectre mac80211 patches that I kept in my tree for the driver. But I now remember that I never published those because of that exact "warning". These lines are coming from there. > >> > --- a/drivers/net/wireless/ath/carl9170/main.c > >> > +++ b/drivers/net/wireless/ath/carl9170/main.c > >> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, > >> > int ret; > >> > > >> > BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); > >> > - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); > >> > + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); > >> > >> IMHO this just makes the code worse. Does it make sense to workaround > >> (stupid) compiler warnings like this? > > > > True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there > > to guard but it's getting compiled away. I could also just drop it. > > Either way is fine for me. If the GCC (by default) emits a warning for > this we need to silence that warning, so in that respect I guess we > should apply this. Unless better solutions come up, of course :) Note: I dropped this patch from patchwork. So, there's nothing that needs to be done now ;) Well, except OT: Would you mind adding the QCA4019 boardfiles that have accumulated over the past six-ish months? <https://www.mail-archive.com/ath10k@lists.infradead.org/msg09966.html> That list already had: ath10k-firmware: QCA4019 hw1.0: Add ASUS Lyra specific BDFs ath10k-firmware: QCA4019 hw1.0: Add Linksys EA6350v3 ath10k-firmware: QCA4019 hw1.0: Add Qxwlan E2600AC specific BDFs ath10k-firmware: QCA4019 hw1.0: Update OpenMesh A62 specific BDFs ath10k-firmware: QCA9888 hw2.0: Update OpenMesh A62 specific BDFs ath10k-firmware: QCA4019 hw1.0: Update OpenMesh A42 specific BDFs ath10k-firmware: QCA4019 hw1.0: Add ALFA Network AP120C-AC specific BDF ath10k-firmware: QCA4019 hw1.0: Add AVM FRITZ!Box 7530 specific BDFs ath10k-firmware: QCA4019 hw1.0: Update Linksys EA6350v3 specific BDFs and now there's even more: + ath10k-firmware: QCA4019 hw1.0: Add Qxwlan E2600AC specific BDFs (Watch out, there are multiple versions! The first ones are dodgy with a bad crc) This should be the right "one" <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10007.html> + ath10k-firmware: QCA4019 hw1.0: Add AVM FRITZ!Repeater 3000 specific BDFs <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10101.html> + ath10k-firmware: IPQ4018 hw1.0: Add EnGenius ENS620EXT specific BDFs <http://lists.infradead.org/pipermail/ath10k/2019-March/013034.html> + ath10k-firmware: QCA9984 hw1.0: Update Netgear Orbi Pro SRK60 specific BDFs ath10k-firmware: QCA4019 hw1.0: Update Netgear Orbi Pro SRK60 specific BDFs One for QCA9984 and one for two QCA4019 <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10170.html> <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10171.html> + ath10k-firmware: IPQ4019 hw1.0: Add BDF for Linksys EA8300 [1/2] ath10k-firmware: QCA9888 hw1.0: Add BDF for Linksys EA8300 [2/2] <http://lists.infradead.org/pipermail/ath10k/2019-May/013403.html> <http://lists.infradead.org/pipermail/ath10k/2019-May/013404.html> Cheers, Christian
Christian Lamparter <chunkeey@gmail.com> writes: > On Tuesday, June 18, 2019 2:11:53 PM CEST Kalle Valo wrote: >> Christian Lamparter <chunkeey@gmail.com> writes: >> > On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote: >> >> Christian Lamparter <chunkeey@gmail.com> writes: >> >> >> >> > This patch fixes a noisy warning triggered by -Wenum-compare >> >> > >> >> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ >> >> > | and ‘enum ar9170_txq’ [-Wenum-compare] >> >> > | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); >> >> > | ^ >> >> > | [...] >> >> > >> >> > This is a little bit unfortunate, since the number of queues >> >> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 >> >> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or >> >> > less defined by the AR9170 hardware. >> >> >> >> Is the warning enabled by default? TBH I'm not seeing how useful this >> >> warning is for kernel development. >> > >> > It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" >> > in the main Makefile). >> > >> > I tried debian's gcc starting from 4.6 to the lastest 8.3. They all >> > complain about it in various degrees. >> > >> > https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options >> >> Ok, odd that I haven't noticed this warning. Maybe I have been just >> blind. > > Sorry. No, I messed up there. I made a patch back in the day during the spectre > mac80211 patches that I kept in my tree for the driver. But I now remember that > I never published those because of that exact "warning". > These lines are coming from there. > >> >> > --- a/drivers/net/wireless/ath/carl9170/main.c >> >> > +++ b/drivers/net/wireless/ath/carl9170/main.c >> >> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, >> >> > int ret; >> >> > >> >> > BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); >> >> > - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); >> >> > + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); >> >> >> >> IMHO this just makes the code worse. Does it make sense to workaround >> >> (stupid) compiler warnings like this? >> > >> > True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there >> > to guard but it's getting compiled away. I could also just drop it. >> >> Either way is fine for me. If the GCC (by default) emits a warning for >> this we need to silence that warning, so in that respect I guess we >> should apply this. Unless better solutions come up, of course :) > > Note: I dropped this patch from patchwork. So, there's nothing that > needs to be done now ;) Thanks, except I would prefer that I drop the patch myself :) In numerous cases I have been wondering there a patch has disappeared and only after a while I realise the submitter deleted the patch altogether (which also destroys the conversation from patchwork etc). IMHO that's really bad UX in patchwork, I should file a bug report about that. So in general it's enough to say "Kalle, please drop the patch" and I'll do the rest. > Well, except OT: Would you mind adding the QCA4019 boardfiles that > have accumulated over the past six-ish months? Yeah, those are in my queue. But I'll first want to implement a script so that I don't need to manually add all of them and I just haven't found the time for that.
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index eaea08581cea..acc881bd7f9f 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, int ret; BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ); - BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); + BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ); mutex_lock(&ar->mutex); memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); ret = carl9170_set_qos(ar);
This patch fixes a noisy warning triggered by -Wenum-compare |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’ | and ‘enum ar9170_txq’ [-Wenum-compare] | BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ); | ^ | [...] This is a little bit unfortunate, since the number of queues (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11 (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or less defined by the AR9170 hardware. Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- drivers/net/wireless/ath/carl9170/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)