Message-ID: <20200817084349.GS4354@dell>
Date:   Mon, 17 Aug 2020 09:43:49 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Christian Lamparter <chunkeey@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org,
        Christian Lamparter <chunkeey@...glemail.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        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

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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists