Re: [PATCH 02/32] powercap: Stop using 32-bit MSR interfaces

[Jürgen Groß]

On 29.06.26 14:31, Ingo Molnar wrote:
> * Jürgen Groß <jgross@xxxxxxxx> wrote:
>
> > On 29.06.26 13:25, Ingo Molnar wrote:
> > > * Juergen Gross <jgross@xxxxxxxx> wrote:
> > >
> > > > The 32-bit MSR interfaces rdmsr_safe() and wrmsr_safe() are planned to
> > > > be removed. Use the related 64-bit variants instead.
> > > >
> > > > Drop a pointless initializer.
> > >
> > > Was the initializer really 'pointless'? IIRC it suppressed a build warning
> > > on 32-bit builds, at least in some past iterations of these APIs.
> >
> > I don't see the connection to 32-bit builds. Either the uninitialized
> > variable is a problem or not. And I don't see why "l" didn't need an initial
> > value (it is modified in this function only via a "&=" operation, so some
> > bits would still be random in case it is seen not to be set by rdmsr_safe()).
> >
> > In reality it should never be a problem, as rdmsr_safe() will always set both
> > output variables to a value (0 in case of error, MSR contents else).
>
> So it's not rdmsr_safe() but rdmsr(), but we had this:
>
>    10a5d65856b9 ("x86/process: Convert rdmsr() to rdmsrq() in arch_post_acpi_subsys_init() to address W=1 warning")
>
> Which addressed this W=1 warning:
>
>    arch/x86/kernel/process.c: In function 'arch_post_acpi_subsys_init':
>    arch/x86/kernel/process.c:972:17: warning: variable 'hi' set but not used
>
> By the pattern the 'pointless' hi=0 initialization looks similar
> and may not be as pointless as it seems. :-)

Maybe I'm not getting something here, but 10a5d65856b9 was not about a
missing initializer, but about "hi" just not being consumed after setting it.
"hi" wasn't initialized before 10a5d65856b9 and it was removed by this commit.

I don't see how a pointless initializer would help in such a case.

It would mean that setting a variable twice fixes a "variable set but not
used" warning, which sounds a little bit weird.

BTW, the "hi=0" initializer was added _before_ rdmsr() would return with the
value being set in case of an error. So at the time of the patch setting hi
to 0 it was correct, but this reason vanished only 2 months later.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature