Re: [PATCH 2/7] drivers/hwmon: add local variable for newly allocated attribute_group**

From: Guenter Roeck <linux@roeck-us.net>
To: Max Kellermann <max.kellermann@ionos.com>
Cc: Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/7] drivers/hwmon: add local variable for newly allocated attribute_group**
Date: Mon, 9 Oct 2023 12:36:38 -0700	[thread overview]
Message-ID: <61bd7bfd-5054-4578-bcb5-0b855706b6d7@roeck-us.net> (raw)
In-Reply-To: <CAKPOu+8Uk4GNpZ3DfWhNWXR1bQ60LxcaK9nKs+kRKuq3BOJtmA@mail.gmail.com>

On Mon, Oct 09, 2023 at 07:28:03PM +0200, Max Kellermann wrote:
> On Mon, Oct 9, 2023 at 7:19 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > I have no idea what this is about, and I don't see how that would
> > improve anything, but ...
> 
> Later, we can make lots of global variables "const", which puts them
> in ".rodata" (write-protected at runtime). This is some
> micro-hardening.
> 
> > CHECK: multiple assignments should be avoided
> > #101: FILE: drivers/hwmon/hwmon.c:794:
> > +               hwdev->groups = new_groups = kcalloc(ngroups, sizeof(*new_groups), GFP_KERNEL);
> 
> What program emitted this warning? checkpatch.pl had no error. I'll
> change it in all patches.

I doubt that you ran checkpatch --strict. That check has existed in checkpatch
at least since 2007.

Also, process/coding-style.rst says:

    Don't put multiple assignments on a single line either.  Kernel coding style
    is super simple.  Avoid tricky expressions.

As far as I know that guildeline has not changed.

Now, you might argue something like "who cares about checkpatch --strict",
but in Documentation/hwmon/submitting-patches.rst we specifically say

* Please run your patch through 'checkpatch --strict'. There should be no
  errors, no warnings, and few if any check messages. If there are any
  messages, please be prepared to explain.

So, please explain why this message and with it the coding style violation
should be ignored.

> 
> > either case, this change is not acceptable.
> 
> Because of the multi-assignment, or is there something else?

I don't really see the benefit of this code, and I am not sure if the
explanation about compiler optimization is really valid. It makes me
want to run some test compliations to see if the claim is really true,
and I really don't have time for that.

Also, as Greg points out, this is not in a hot code path but executed
exactly once for each hwmon device, so making such a change with a reason
like that just invites lots of follow-up patches with similar reasons,
and then the submitters of those can point to this patch and argue
"but you accepted that one". You say "micro-hardening" above, but for
me it is time consuming micro-optimization.

Guenter