lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Aug 2020 10:54:21 +0300 From: Kalle Valo <kvalo@...eaurora.org> To: Lee Jones <lee.jones@...aro.org> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>, Christian Lamparter <chunkeey@...il.com>, davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org, Christian Lamparter <chunkeey@...glemail.com>, Johannes Berg <johannes@...solutions.net>, linux-wireless@...r.kernel.org, netdev@...r.kernel.org Subject: Re: [PATCH 08/30] net: wireless: ath: carl9170: Mark 'ar9170_qmap' as __maybe_unused Lee Jones <lee.jones@...aro.org> writes: > On Mon, 17 Aug 2020, Kalle Valo wrote: > >> Rasmus Villemoes <linux@...musvillemoes.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[]" -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Powered by blists - more mailing lists