Re: [PATCH v2 1/3] base: mark 'no_warn' as unused

[Bill Wendling]

On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote:
> > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote:
> > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c
> > > > respectively, drop everyone else.
> > >
> > > Odd no one cc:ed us originally, I guess they didn't want the patch ever
> > > merged?  :(
> > >
I don't believe I saw you or Rafael listed in the
"script/get_maintainers" output. I tried to copy everyone who showed
up.

> > > >
> > > > Original post:
> > > >
> > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@xxxxxxxxxx/
> > > >
> > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote:
> > > > > Fix the following build warning:
> > > > >
> > > > >    drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable]
> > > > >          int no_warn;
> > >
> > > That's not going to be a good warning to ever have the kernel use due to
> > > how lots of hardware works (i.e. we need to do a read after a write but
> > > we can throw the read away as it does not matter).
> > >
> > >
> > > > >
> > > > > This variable is used to remove another warning, but causes a warning
> > > > > itself. Mark it as 'unused' to avoid that.
> > > > >
> > > > > Signed-off-by: Bill Wendling <morbo@xxxxxxxxxx>
> > > >
> > > > Even though they evaluate to the same thing, it might be worth using
> > > > "__always_unused" here because it is :)
> > >
> > > But it is not unused, the value is written into it.
> > >
> > I believe that only matters if the variable is marked "volatile".
>
> "volatile" means nothing anymore, never use it or even think about it
> again please :)
>
Never use what? ;-)

> > Otherwise, the variable itself is never used. A "variable that's
> > written to but not read from," in fact, is the whole reason for the
> > warning.
>
> But that is ok!  Sometimes you need to do this with hardware (like all
> PCI devices).  This is a legitimate code flow for many hardware types
> and if a C compiler thinks that this is not ok, then it is broken.
>
Well, no. A C compiler cares about the C language. A variable that's
assigned to but not otherwise used isn't useful in the language. Like
most warnings, the compiler warns because these situations have led to
errors in the past (e.g. maybe someone assigned to the wrong variable
because they mistyped the name or something). So this is a perfectly
valid warning for a C compiler to emit. This especially holds true
when the function being called is marked as requiring the return value
to be checked, as is the case with the functions whose values are
assigned 'no_warn'.

> So be VERY careful when changing drivers based on this warning.  Because
> of this, I do not think you can enable it over the whole kernel without
> causing major problems in some areas.
>
> But that is independent of this specific issue you are trying to patch
> here, I say this to warn you of a number of stupid places where people
> have tried to "optimize away" reads based on this compiler warning in
> drivers, and we have had to add them back because it broke
> functionality.
>
I definitely agree that we shouldn't blindly remove code just because
the return value assigned to a variable isn't used. That's not what
this patch is doing.

> > > So this isn't ok, sometimes we want to write to variables but never care
> > > about the value, that does not mean the compiler should complain about
> > > it.
> > >
> > Typically, if you don't care about the return value, you simply don't
> > assign it to a variable (cf. printf). However, the functions that
> > assign to "no_warn" have the "warn_unused_result" attribute. The fact
> > that the variable is named "no_warn" seems to indicate that it's meant
> > to remain unused, even if it probably should be checked.
>
> These functions have warn_unused_result set on them because for 99% of
> the time, I want the value to be checked.  But as you can see in this
> use, as per the comments in the code, we do not care about the result
> for a very good reason.  So we just assign it to a variable to make the
> compiler quiet.
>
Right. And because you[1] had to hack around that warning, it led to
another warning. This is typical with hacks.

[1] Okay, maybe not *you* explicitly, but "you" in the general sense
of "the person who wrote this code."

> > Would you rather the warning be turned off on some level?
>
> Which warning?
>
The one this patch is for.

> The code here, as-is, is correct.  We already have 1 compiler warning
> work around in place, do you want to add another one?  How many can we
> stack on top of each other?
>
This one change should suffice.

> And again, why did you not cc: the maintainers of this code for this
> change?  That's not good...
>
I guess the maintainers didn't show up in the scripts/get_maintainers
list when I ran it. I CC'ed everyone that did show up. There's a long
list of emails in the "To" section. I don't know what else to say...

-bw