Re: [PATCH 08/30] net: wireless: ath: carl9170: Mark 'ar9170_qmap' as __maybe_unused

From: Lee Jones <lee.jones@linaro.org>
To: Kalle Valo <kvalo@codeaurora.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Christian Lamparter <chunkeey@gmail.com>,
	davem@davemloft.net, kuba@kernel.org,
	linux-kernel@vger.kernel.org,
	Christian Lamparter <chunkeey@googlemail.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH 08/30] net: wireless: ath: carl9170: Mark 'ar9170_qmap' as __maybe_unused
Date: Tue, 18 Aug 2020 10:50:24 +0100	[thread overview]
Message-ID: <20200818095024.GZ4354@dell> (raw)
In-Reply-To: <87eeo5mnr0.fsf@codeaurora.org>

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 [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog