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

  This allows the compiler to keep the pointer in a register and
prepares for making the struct field "const".

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 drivers/hwmon/hwmon.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
  

On Mon, Oct 09, 2023 at 06:57:35PM +0200, Max Kellermann wrote:
> This allows the compiler to keep the pointer in a register and
> prepares for making the struct field "const".
> 
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>

I have no idea what this is about, and I don't see how that would
improve anything, but ...

> ---
>  drivers/hwmon/hwmon.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index c7dd3f5b2bd5..e50ab229b27d 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -783,6 +783,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  	hdev = &hwdev->dev;
>  
>  	if (chip) {
> +		const struct attribute_group **new_groups;
>  		struct attribute **attrs;
>  		int ngroups = 2; /* terminating NULL plus &hwdev->groups */
>  
> @@ -790,8 +791,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  			for (i = 0; groups[i]; i++)
>  				ngroups++;
>  
> -		hwdev->groups = kcalloc(ngroups, sizeof(*groups), GFP_KERNEL);
> -		if (!hwdev->groups) {
> +		hwdev->groups = new_groups = kcalloc(ngroups, sizeof(*new_groups), GFP_KERNEL);

CHECK: multiple assignments should be avoided
#101: FILE: drivers/hwmon/hwmon.c:794:
+		hwdev->groups = new_groups = kcalloc(ngroups, sizeof(*new_groups), GFP_KERNEL);

either case, this change is not acceptable.

Guenter
  

On Mon, Oct 09, 2023 at 06:57:35PM +0200, Max Kellermann wrote:
> This allows the compiler to keep the pointer in a register and

Maybe, maybe not, there's no guarantee for register usage.

And it doesn't matter, this is a very slow path, no registers are
required :)

> prepares for making the struct field "const".

What struct field?


> 
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
>  drivers/hwmon/hwmon.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index c7dd3f5b2bd5..e50ab229b27d 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -783,6 +783,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  	hdev = &hwdev->dev;
>  
>  	if (chip) {
> +		const struct attribute_group **new_groups;
>  		struct attribute **attrs;
>  		int ngroups = 2; /* terminating NULL plus &hwdev->groups */
>  
> @@ -790,8 +791,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  			for (i = 0; groups[i]; i++)
>  				ngroups++;
>  
> -		hwdev->groups = kcalloc(ngroups, sizeof(*groups), GFP_KERNEL);
> -		if (!hwdev->groups) {
> +		hwdev->groups = new_groups = kcalloc(ngroups, sizeof(*new_groups), GFP_KERNEL);

So you have a const pointer AND a non-const pointer pointing to the same
thing?


> +		if (!new_groups) {
>  			err = -ENOMEM;
>  			goto free_hwmon;
>  		}
> @@ -804,14 +805,14 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  
>  		hwdev->group.attrs = attrs;
>  		ngroups = 0;
> -		hwdev->groups[ngroups++] = &hwdev->group;
> +		new_groups[ngroups++] = &hwdev->group;

This shoul be identical, you assign both above the same way, so why
change this?

>  
>  		if (groups) {
>  			for (i = 0; groups[i]; i++)
> -				hwdev->groups[ngroups++] = groups[i];
> +				new_groups[ngroups++] = groups[i];

Same here.

thanks,

greg k-h
  

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.

> either case, this change is not acceptable.

Because of the multi-assignment, or is there something else?
  

On Mon, Oct 9, 2023 at 7:27 PM Greg KH <greg@kroah.com> wrote:
>
> On Mon, Oct 09, 2023 at 06:57:35PM +0200, Max Kellermann wrote:
> > This allows the compiler to keep the pointer in a register and
>
> Maybe, maybe not, there's no guarantee for register usage.

Agree. But without this change, register usage is effectively
prevented (much of the time). With this change, there is a chance for
better code. Yeah, it's not a critical code path, and it's only a tiny
tiny optimization - but the real reason for this patch is ....

> > -             hwdev->groups = kcalloc(ngroups, sizeof(*groups), GFP_KERNEL);
> > -             if (!hwdev->groups) {
> > +             hwdev->groups = new_groups = kcalloc(ngroups, sizeof(*new_groups), GFP_KERNEL);
>
> So you have a const pointer AND a non-const pointer pointing to the same
> thing?

.... so I can make "hwdev->groups" const, which wouldn't allow
modifying it. (This isn't obvious in this one patch, but a later patch
in the series does this.)

There are only few functions which allocate a new pointer-array
dynamically; all others are global variables which currently cannot be
"const". This patch set does some lifting to allow adding "const".
  

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