| Message ID | 20200814113933.1903438-9-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | None | expand |
On 2020-08-14 13:39, Lee Jones wrote: > 'ar9170_qmap' is used in some source files which include carl9170.h, > but not all of them. Mark it as __maybe_unused to show that this is > not only okay, it's expected. > > Fixes the following W=1 kernel build warning(s) Is this W=1 really a "must" requirement? I find it strange having __maybe_unused in header files as this "suggests" that the definition is redundant. > from drivers/net/wireless/ath/carl9170/carl9170.h:57, > In file included from drivers/net/wireless/ath/carl9170/carl9170.h:57, > drivers/net/wireless/ath/carl9170/carl9170.h:71:17: warning: ‘ar9170_qmap’ defined but not used [-Wunused-const-variable=] > > > Cc: Christian Lamparter <chunkeey@googlemail.com> > Cc: Kalle Valo <kvalo@codeaurora.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Johannes Berg <johannes@sipsolutions.net> > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/net/wireless/ath/carl9170/carl9170.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h > index 237d0cda1bcb0..9d86253081bce 100644 > --- a/drivers/net/wireless/ath/carl9170/carl9170.h > +++ b/drivers/net/wireless/ath/carl9170/carl9170.h > @@ -68,7 +68,7 @@ > > #define PAYLOAD_MAX (CARL9170_MAX_CMD_LEN / 4 - 1) > > -static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; > +static const u8 __maybe_unused ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; > > #define CARL9170_MAX_RX_BUFFER_SIZE 8192 > >
On Fri, 14 Aug 2020, Christian Lamparter wrote: > On 2020-08-14 13:39, Lee Jones wrote: > > 'ar9170_qmap' is used in some source files which include carl9170.h, > > but not all of them. Mark it as __maybe_unused to show that this is > > not only okay, it's expected. > > > > Fixes the following W=1 kernel build warning(s) > > Is this W=1 really a "must" requirement? I find it strange having Clean W=1 warnings is the dream, yes. I would have thought most Maintainers would be on-board with this. The ones I've worked with thus far have certainly been thankful. Many had this on their own TODO lists. > __maybe_unused in header files as this "suggests" that the > definition is redundant. Not true. If it were redundant then we would remove the line entirely. > > from drivers/net/wireless/ath/carl9170/carl9170.h:57, > > In file included from drivers/net/wireless/ath/carl9170/carl9170.h:57, > > drivers/net/wireless/ath/carl9170/carl9170.h:71:17: warning: ‘ar9170_qmap’ defined but not used [-Wunused-const-variable=] > > > > Cc: Christian Lamparter <chunkeey@googlemail.com> > > Cc: Kalle Valo <kvalo@codeaurora.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Johannes Berg <johannes@sipsolutions.net> > > Cc: linux-wireless@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/net/wireless/ath/carl9170/carl9170.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h > > index 237d0cda1bcb0..9d86253081bce 100644 > > --- a/drivers/net/wireless/ath/carl9170/carl9170.h > > +++ b/drivers/net/wireless/ath/carl9170/carl9170.h > > @@ -68,7 +68,7 @@ > > #define PAYLOAD_MAX (CARL9170_MAX_CMD_LEN / 4 - 1) > > -static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; > > +static const u8 __maybe_unused ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; > > #define CARL9170_MAX_RX_BUFFER_SIZE 8192 > > >
On 2020-08-14 18:40, Lee Jones wrote: > On Fri, 14 Aug 2020, Christian Lamparter wrote: > >> On 2020-08-14 13:39, Lee Jones wrote: >>> 'ar9170_qmap' is used in some source files which include carl9170.h, >>> but not all of them. Mark it as __maybe_unused to show that this is >>> not only okay, it's expected. >>> >>> Fixes the following W=1 kernel build warning(s) >> >> Is this W=1 really a "must" requirement? I find it strange having > > Clean W=1 warnings is the dream, yes. But is it a requirement? > > I would have thought most Maintainers would be on-board with this. From what I know: It's no changes For changes' sake. Because otherwise this would be pretty broken for maintainers. They could just write and revert the same code over and over to prob up their LOC and commit counter. Wouldn't you agree there? > > The ones I've worked with thus far have certainly been thankful. Many > had this on their own TODO lists. Question is, do you really want to be just the cleanup crew there? Since semantic patches came along and a lot of this has been automated. I'm of course after something else. Like: "Isn't there a better way than manually slapping __maybe_unused there to suppress the warning and call it a day?" If you already went down these avenues and can confirm that there's no alternative than this, then "fine". But if there is a better method of doing this, then "let's go with that!". > >> __maybe_unused in header files as this "suggests" that the >> definition is redundant. > > Not true. > > If it were redundant then we would remove the line entirely. So, why adding __maybe_unused then? I find it not very helpful to tell the compiler to "shut up" when you want it's opinion... This was the vibe I got from gcc's attribute unused help text. Cheers, Christian > >>> from drivers/net/wireless/ath/carl9170/carl9170.h:57, >>> In file included from drivers/net/wireless/ath/carl9170/carl9170.h:57, >>> drivers/net/wireless/ath/carl9170/carl9170.h:71:17: warning: ‘ar9170_qmap’ defined but not used [-Wunused-const-variable=] >>> >>> Cc: Christian Lamparter <chunkeey@googlemail.com> >>> Cc: Kalle Valo <kvalo@codeaurora.org> >>> Cc: "David S. Miller" <davem@davemloft.net> >>> Cc: Jakub Kicinski <kuba@kernel.org> >>> Cc: Johannes Berg <johannes@sipsolutions.net> >>> Cc: linux-wireless@vger.kernel.org >>> Cc: netdev@vger.kernel.org >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/net/wireless/ath/carl9170/carl9170.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h >>> index 237d0cda1bcb0..9d86253081bce 100644 >>> --- a/drivers/net/wireless/ath/carl9170/carl9170.h >>> +++ b/drivers/net/wireless/ath/carl9170/carl9170.h >>> @@ -68,7 +68,7 @@ >>> #define PAYLOAD_MAX (CARL9170_MAX_CMD_LEN / 4 - 1) >>> -static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; >>> +static const u8 __maybe_unused ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; >>> #define CARL9170_MAX_RX_BUFFER_SIZE 8192 >>> >> >
On 14/08/2020 17.14, Christian Lamparter wrote: > On 2020-08-14 13:39, Lee Jones wrote: >> 'ar9170_qmap' is used in some source files which include carl9170.h, >> but not all of them. Mark it as __maybe_unused to show that this is >> not only okay, it's expected. >> >> Fixes the following W=1 kernel build warning(s) > > Is this W=1 really a "must" requirement? I find it strange having > __maybe_unused in header files as this "suggests" that the > definition is redundant. In this case it seems one could replace the table lookup with a static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; } gcc doesn't warn about unused static inline functions (or one would have a million warnings to deal with). Just my $0.02. Rasmus
On Fri, 14 Aug 2020, Christian Lamparter wrote: > On 2020-08-14 18:40, Lee Jones wrote: > > On Fri, 14 Aug 2020, Christian Lamparter wrote: > > > > > On 2020-08-14 13:39, Lee Jones wrote: > > > > 'ar9170_qmap' is used in some source files which include carl9170.h, > > > > but not all of them. Mark it as __maybe_unused to show that this is > > > > not only okay, it's expected. > > > > > > > > Fixes the following W=1 kernel build warning(s) > > > > > > Is this W=1 really a "must" requirement? I find it strange having > > > > Clean W=1 warnings is the dream, yes. > But is it a requirement? Depends how you define a requirement. This is required to successfully and usefully enable W=1 warnings in our testing builds without being overloaded with current issues. Something I know a great number of maintainers have been either trying to do, or at least wanting to do for a long time. Being able to enable W=1 builds at the subsystem level is extremely helpful in order to keep the kernel clean(er). As most subsystems don't (can't) have them enabled presently (due to being overwhelmed) they will likely creep/increase. So far, the following subsystems have been on-board with this (and are now clean, or very nearly clean, as a result): ASoC backlight cpufreq dmaengine gpio hwmon iio mfd misc mmc pinctrl pwm regulator remoteproc scsi spi usb video > > I would have thought most Maintainers would be on-board with this. > From what I know: It's no changes For changes' sake. Because otherwise this > would be pretty broken for maintainers. They could just write and revert the > same code over and over to prob up their LOC and commit counter. Wouldn't > you agree there? I don't agree at all. Why would anyone revert a fix? That act would be intentionally add a warning? A fools errand I think. > > The ones I've worked with thus far have certainly been thankful. Many > > had this on their own TODO lists. > Question is, do you really want to be just the cleanup crew there? Since > semantic patches came along and a lot of this has been automated. I'm happy to put in the work. Most people have been very grateful. If this work can be automated, than that would be wonderful. However, 18000 warnings (now down to 15000) tell me that this can not be the case. > I'm of course after something else. Like: "Isn't there a better way than > manually slapping __maybe_unused there to suppress the warning and call it a > day?" If you already went down these avenues and can confirm that there's no > alternative than this, then "fine". But if there is a better > method of doing this, then "let's go with that!". So for these kinds of issues we have a choice. In order of preference: 1. Genuinely unused; remove it 2. Used in a single location; move it to that location 3. Used in multiple, but not all locations: a. Mark as __maybe_unused b. Locate or create a special header file between users b. Duplicate it and place it in all used locations I went for 3a here, as 1 and 2 aren't valid. > > > __maybe_unused in header files as this "suggests" that the > > > definition is redundant. > > > > Not true. > > > > If it were redundant then we would remove the line entirely. > So, why adding __maybe_unused then? I find it not very helpful to > tell the compiler to "shut up" when you want it's opinion... > This was the vibe I got from gcc's attribute unused help text. Effectively, you're telling the compiler that it's not correct in certain circumstances, like this one. Here the variable is being used, but not by all users who share the header file. In other valid cases of its use the variable may only be used when a given CONFIG_* is enabled. __always_unused however does just tell the compiler to shut-up. :) If you have any better solutions, other than to let the compiler spout useless warnings which taint the build log and hide other, more important issues, then I'd be happy to hear them.
Rasmus Villemoes <linux@rasmusvillemoes.dk> writes: > On 14/08/2020 17.14, Christian Lamparter wrote: >> On 2020-08-14 13:39, Lee Jones wrote: >>> 'ar9170_qmap' is used in some source files which include carl9170.h, >>> but not all of them. Mark it as __maybe_unused to show that this is >>> not only okay, it's expected. >>> >>> Fixes the following W=1 kernel build warning(s) >> >> Is this W=1 really a "must" requirement? I find it strange having >> __maybe_unused in header files as this "suggests" that the >> definition is redundant. > > In this case it seems one could replace the table lookup with a > > static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; } > > gcc doesn't warn about unused static inline functions (or one would have > a million warnings to deal with). Just my $0.02. Yeah, this is much better. And I think that static variables should not even be in the header files. Doesn't it mean that there's a local copy of the variable everytime the .h file is included? Sure, in this case the overhead is small (4 bytes per include) but still it's wrong. Having a static inline function would solve that problem as well the compiler warning.
On 2020-08-17 14:59, Kalle Valo wrote: > Rasmus Villemoes <linux@rasmusvillemoes.dk> writes: > >> On 14/08/2020 17.14, Christian Lamparter wrote: >>> On 2020-08-14 13:39, Lee Jones wrote: >>>> 'ar9170_qmap' is used in some source files which include carl9170.h, >>>> but not all of them. Mark it as __maybe_unused to show that this is >>>> not only okay, it's expected. >>>> >>>> Fixes the following W=1 kernel build warning(s) >>> >>> Is this W=1 really a "must" requirement? I find it strange having >>> __maybe_unused in header files as this "suggests" that the >>> definition is redundant. >> >> In this case it seems one could replace the table lookup with a >> >> static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; } >> >> gcc doesn't warn about unused static inline functions (or one would have >> a million warnings to deal with). Just my $0.02. > > Yeah, this is much better. Yes, this is much better than just adding __maybe_unused :). To be on the safe side (and to get rid of a & in tx.c:666), the "3 - idx" should be something like "return (3 - idx) & CARL9170_TX_STATUS_QUEUE". I think its also possible to just use clamp_t (or min_t since the u8 has no negative values) or make use of a switch statement [analogues what was done in ath10k commit: 91493e8e10 "ath10k: fix recent bandwidth conversion bug" (just to be clear. Yes this ath10k commit has nothing to do with queues, but it is a nice, atomic switch-case static inline function example).] > And I think that static variables should not even be in the header > files. Doesn't it mean that there's a local copy of the variable > everytime the .h file is included? Sure, in this case the overhead is > small (4 bytes per include) but still it's wrong. Seems to be "sort of". I compiled both, the current vanilla carl9170 and with Lee Jones' patch on Debian's current gcc 10.2.0 (Debian 10.2.0-5) and gnu ld 2.35. The ar9170_qmap symbol is only present in the object file if ar9170_qmap is used by the source. In the final module file (carl9170.ko), there are two ar9170_qmap symboles in the module's .rodata section (one is coming from main.o code and the other from tx.o). (The use of __maybe_unused didn't make any difference. Same overall section and file sizes). Cheers, Christian
On Mon, 17 Aug 2020, Kalle Valo wrote: > Rasmus Villemoes <linux@rasmusvillemoes.dk> writes: > > > On 14/08/2020 17.14, Christian Lamparter wrote: > >> On 2020-08-14 13:39, Lee Jones wrote: > >>> 'ar9170_qmap' is used in some source files which include carl9170.h, > >>> but not all of them. Mark it as __maybe_unused to show that this is > >>> not only okay, it's expected. > >>> > >>> Fixes the following W=1 kernel build warning(s) > >> > >> Is this W=1 really a "must" requirement? I find it strange having > >> __maybe_unused in header files as this "suggests" that the > >> definition is redundant. > > > > In this case it seems one could replace the table lookup with a > > > > static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; } > > > > gcc doesn't warn about unused static inline functions (or one would have > > a million warnings to deal with). Just my $0.02. > > Yeah, this is much better. > > And I think that static variables should not even be in the header > files. Doesn't it mean that there's a local copy of the variable > everytime the .h file is included? Sure, in this case the overhead is > small (4 bytes per include) but still it's wrong. It happens a lot. As I stated before, the 2 viable options are to a) move it into the source files; ensuring code duplication, unnecessary maintenance burden and probably disparity over time, or b) create (or locate if there is one already) a special header file which is only to be included by the users. The later option gets really complicated if there are a variety of tables which are included by any given number of source file permutations. The accepted answer in all of the other subsystems I've worked with so far, is to use __maybe_unused. It's simple, non-intrusive and doesn't rely on any functional changes. > Having a static inline > function would solve that problem as well the compiler warning. This time yes, but it's a hack that will only work with simple, linear data. Try doing that with some of the other, more complicated tables, like mwifiex_sdio_sd8*.
Lee Jones <lee.jones@linaro.org> writes: > On Mon, 17 Aug 2020, Kalle Valo wrote: > >> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes: >> >> > On 14/08/2020 17.14, Christian Lamparter wrote: >> >> On 2020-08-14 13:39, Lee Jones wrote: >> >>> 'ar9170_qmap' is used in some source files which include carl9170.h, >> >>> but not all of them. Mark it as __maybe_unused to show that this is >> >>> not only okay, it's expected. >> >>> >> >>> Fixes the following W=1 kernel build warning(s) >> >> >> >> Is this W=1 really a "must" requirement? I find it strange having >> >> __maybe_unused in header files as this "suggests" that the >> >> definition is redundant. >> > >> > In this case it seems one could replace the table lookup with a >> > >> > static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; } >> > >> > gcc doesn't warn about unused static inline functions (or one would have >> > a million warnings to deal with). Just my $0.02. >> >> Yeah, this is much better. >> >> And I think that static variables should not even be in the header >> files. Doesn't it mean that there's a local copy of the variable >> everytime the .h file is included? Sure, in this case the overhead is >> small (4 bytes per include) but still it's wrong. > > It happens a lot. > > As I stated before, the 2 viable options are to a) move it into the > source files; ensuring code duplication, unnecessary maintenance > burden and probably disparity over time, or b) create (or locate if > there is one already) a special header file which is only to be > included by the users. > > The later option gets really complicated if there are a variety of > tables which are included by any given number of source file > permutations. > > The accepted answer in all of the other subsystems I've worked with so > far, is to use __maybe_unused. It's simple, non-intrusive and doesn't > rely on any functional changes. > >> Having a static inline >> function would solve that problem as well the compiler warning. > > This time yes, but it's a hack that will only work with simple, > linear data. To me __maybe_unused is a hack and a static inline function is a much better solution. > Try doing that with some of the other, more complicated > tables, like mwifiex_sdio_sd8*. Then the table should be moved to a .c file and the .h file should have "extern const int foo[]"
diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h index 237d0cda1bcb0..9d86253081bce 100644 --- a/drivers/net/wireless/ath/carl9170/carl9170.h +++ b/drivers/net/wireless/ath/carl9170/carl9170.h @@ -68,7 +68,7 @@ #define PAYLOAD_MAX (CARL9170_MAX_CMD_LEN / 4 - 1) -static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; +static const u8 __maybe_unused ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 }; #define CARL9170_MAX_RX_BUFFER_SIZE 8192
'ar9170_qmap' is used in some source files which include carl9170.h, but not all of them. Mark it as __maybe_unused to show that this is not only okay, it's expected. Fixes the following W=1 kernel build warning(s) from drivers/net/wireless/ath/carl9170/carl9170.h:57, In file included from drivers/net/wireless/ath/carl9170/carl9170.h:57, drivers/net/wireless/ath/carl9170/carl9170.h:71:17: warning: ‘ar9170_qmap’ defined but not used [-Wunused-const-variable=] NB: Snipped - lots of these repeat Cc: Christian Lamparter <chunkeey@googlemail.com> Cc: Kalle Valo <kvalo@codeaurora.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Johannes Berg <johannes@sipsolutions.net> Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/net/wireless/ath/carl9170/carl9170.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)