[PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Valdis Klētnieks]

We get a warning when building with W=1:

  CC      arch/x86/kernel/cpu/umwait.o
arch/x86/kernel/cpu/umwait.c: In function 'umwait_init':
arch/x86/kernel/cpu/umwait.c:183:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
  183 |  int ret;
      |      ^~~

And indeed, we don't do anything with it, so clean it  up.

Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>

diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c
index 6a204e7336c1..3d1d3952774a 100644
--- a/arch/x86/kernel/cpu/umwait.c
+++ b/arch/x86/kernel/cpu/umwait.c
@@ -180,12 +180,11 @@ static struct attribute_group umwait_attr_group = {
 static int __init umwait_init(void)
 {
 	struct device *dev;
-	int ret;
 
 	if (!boot_cpu_has(X86_FEATURE_WAITPKG))
 		return -ENODEV;
 
-	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online",
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online",
 				umwait_cpu_online, NULL);
 
 	register_syscore_ops(&umwait_syscore_ops);

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

Valdis,

On Thu, 8 Aug 2019, Valdis Klētnieks wrote:

I really appreciate your work, but can you please refrain from using file
names as prefixes?

git log $FILE gives you usually a pretty good hint what the proper prefix
is:

  bd9a0c97e53c ("x86/umwait: Add sysfs interface to control umwait maximum time")
  ff4b353f2ef9 ("x86/umwait: Add sysfs interface to control umwait C0.2 state")
  bd688c69b7e6 ("x86/umwait: Initialize umwait control values")

See?

> We get a warning when building with W=1:

Please avoid 'We/I' in changelogs.
 
>   CC      arch/x86/kernel/cpu/umwait.o
> arch/x86/kernel/cpu/umwait.c: In function 'umwait_init':
> arch/x86/kernel/cpu/umwait.c:183:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
>   183 |  int ret;
>       |      ^~~
> 
> And indeed, we don't do anything with it, so clean it  up.

Well, the question is whether removing the variable is the right thing to
do.
 
> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> 
> diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c
> index 6a204e7336c1..3d1d3952774a 100644
> --- a/arch/x86/kernel/cpu/umwait.c
> +++ b/arch/x86/kernel/cpu/umwait.c
> @@ -180,12 +180,11 @@ static struct attribute_group umwait_attr_group = {
>  static int __init umwait_init(void)
>  {
>  	struct device *dev;
> -	int ret;
>  
>  	if (!boot_cpu_has(X86_FEATURE_WAITPKG))
>  		return -ENODEV;
>  
> -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online",
> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online",
>  				umwait_cpu_online, NULL);

If that fails then umwait is broken. So instead of removing it, this should
actually check the return code and act accordingly. Fenghua?
  
>  	register_syscore_ops(&umwait_syscore_ops);

Thanks,

	tglx

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Valdis Klētnieks]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Thu, 08 Aug 2019 22:04:03 +0200, Thomas Gleixner said:

> I really appreciate your work, but can you please refrain from using file
> names as prefixes?

OK, will do so going forward..

> > We get a warning when building with W=1:
>
> Please avoid 'We/I' in changelogs.

OK..

> >   CC      arch/x86/kernel/cpu/umwait.o
> > arch/x86/kernel/cpu/umwait.c: In function 'umwait_init':
> > arch/x86/kernel/cpu/umwait.c:183:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
> >   183 |  int ret;
> >       |      ^~~
> >
> > And indeed, we don't do anything with it, so clean it  up.
>
> Well, the question is whether removing the variable is the right thing to
> do.

> If that fails then umwait is broken. So instead of removing it, this should
> actually check the return code and act accordingly. Fenghua?

[/usr/src/linux-next] git grep umwait_init
arch/x86/kernel/cpu/umwait.c:static int __init umwait_init(void)
arch/x86/kernel/cpu/umwait.c:device_initcall(umwait_init);

It isn't clear that whatever is doing the device_initcall()s will be able to do
any reasonable recovery if we return an error, so any error recovery is going
to have to happen before the function returns. It might make sense to do an
'if (ret) return;' before going further in the function, but given the comment a
few lines further down about ignoring errors,  it was apparently considered
more important to struggle through and register stuff in sysfs even if umwait
was broken....


[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

Valdis,

On Thu, 8 Aug 2019, Valdis Klētnieks wrote:
> On Thu, 08 Aug 2019 22:04:03 +0200, Thomas Gleixner said:
> 
> > I really appreciate your work, but can you please refrain from using file
> > names as prefixes?
> 
> OK, will do so going forward..

Care to resend the last few with fixed subject lines, so I don't have to
clean them up?
 
> > > And indeed, we don't do anything with it, so clean it  up.
> >
> > Well, the question is whether removing the variable is the right thing to
> > do.
> 
> > If that fails then umwait is broken. So instead of removing it, this should
> > actually check the return code and act accordingly. Fenghua?
> 
> [/usr/src/linux-next] git grep umwait_init
> arch/x86/kernel/cpu/umwait.c:static int __init umwait_init(void)
> arch/x86/kernel/cpu/umwait.c:device_initcall(umwait_init);
> 
> It isn't clear that whatever is doing the device_initcall()s will be able to do
> any reasonable recovery if we return an error, so any error recovery is going
> to have to happen before the function returns. It might make sense to do an
> 'if (ret) return;' before going further in the function, but given the comment a
> few lines further down about ignoring errors,  it was apparently considered
> more important to struggle through and register stuff in sysfs even if umwait
> was broken....

I missed that when going through it.

The right thing to do is to have a cpu_offline callback which clears the
umwait MSR. That covers also the failure in the cpu hotplug setup. Then
handling an error return makes sense and keeps everything in a workable
shape.

Thanks,

	tglx

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Valdis Klētnieks]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Thu, 08 Aug 2019 22:32:38 +0200, Thomas Gleixner said:

> Care to resend the last few with fixed subject lines, so I don't have to
> clean them up?

Will do that later this evening...

> The right thing to do is to have a cpu_offline callback which clears the
> umwait MSR. That covers also the failure in the cpu hotplug setup. Then
> handling an error return makes sense and keeps everything in a workable
> shape.

OK, in that case, toss the umwait patch for now....

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Fenghua Yu]

On Thu, Aug 08, 2019 at 10:32:38PM +0200, Thomas Gleixner wrote:
> Valdis,
> 
> On Thu, 8 Aug 2019, Valdis Klētnieks wrote:
> > On Thu, 08 Aug 2019 22:04:03 +0200, Thomas Gleixner said: It isn't 
> > clear that whatever is doing the device_initcall()s will be able to 
> > do any reasonable recovery if we return an error, so any error 
> > recovery is going to have to happen before the function returns. It 
> > might make sense to do an 'if (ret) return;' before going further in 
> > the function, but given the comment a few lines further down about 
> > ignoring errors, it was apparently considered more important to 
> > struggle through and register stuff in sysfs even if umwait was 
> > broken....
> 
> I missed that when going through it.
> 
> The right thing to do is to have a cpu_offline callback which clears 
> the umwait MSR. That covers also the failure in the cpu hotplug setup. 
> Then handling an error return makes sense and keeps everything in a 
> workable shape.

When cpu is offline, the MSR won't be used. We don't need to clear it, right?

Thanks.

-Fenghua

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

On Thu, 8 Aug 2019, Fenghua Yu wrote:
> On Thu, Aug 08, 2019 at 10:32:38PM +0200, Thomas Gleixner wrote:
> > Valdis,
> > 
> > On Thu, 8 Aug 2019, Valdis Klētnieks wrote:
> > > On Thu, 08 Aug 2019 22:04:03 +0200, Thomas Gleixner said: It isn't 
> > > clear that whatever is doing the device_initcall()s will be able to 
> > > do any reasonable recovery if we return an error, so any error 
> > > recovery is going to have to happen before the function returns. It 
> > > might make sense to do an 'if (ret) return;' before going further in 
> > > the function, but given the comment a few lines further down about 
> > > ignoring errors, it was apparently considered more important to 
> > > struggle through and register stuff in sysfs even if umwait was 
> > > broken....
> > 
> > I missed that when going through it.
> > 
> > The right thing to do is to have a cpu_offline callback which clears 
> > the umwait MSR. That covers also the failure in the cpu hotplug setup. 
> > Then handling an error return makes sense and keeps everything in a 
> > workable shape.
> 
> When cpu is offline, the MSR won't be used. We don't need to clear it, right?

Groan. If soemthing goes wrong when registering the hotplug callback, what
undoes the MSR setup which might have happened and what takes care of it on
cpus coming online later? Exactly nothing. Then you have a non-consistent
behaviour.

Make stuff symmmetric and correct and not optimized for the sunshine case.

Thanks,

	tglx

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Fenghua Yu]

On Fri, Aug 09, 2019 at 11:49:49AM +0200, Thomas Gleixner wrote:
> On Thu, 8 Aug 2019, Fenghua Yu wrote:
> > On Thu, Aug 08, 2019 at 10:32:38PM +0200, Thomas Gleixner wrote:
> > > Valdis,
> > > 
> > > On Thu, 8 Aug 2019, Valdis Klētnieks wrote:
> > > > On Thu, 08 Aug 2019 22:04:03 +0200, Thomas Gleixner said: It isn't 
> > > > clear that whatever is doing the device_initcall()s will be able to 
> > > > do any reasonable recovery if we return an error, so any error 
> > > > recovery is going to have to happen before the function returns. It 
> > > > might make sense to do an 'if (ret) return;' before going further in 
> > > > the function, but given the comment a few lines further down about 
> > > > ignoring errors, it was apparently considered more important to 
> > > > struggle through and register stuff in sysfs even if umwait was 
> > > > broken....
> > > 
> > > I missed that when going through it.
> > > 
> > > The right thing to do is to have a cpu_offline callback which clears 
> > > the umwait MSR. That covers also the failure in the cpu hotplug setup. 
> > > Then handling an error return makes sense and keeps everything in a 
> > > workable shape.
> > 
> > When cpu is offline, the MSR won't be used. We don't need to clear it, right?
> 
> Groan. If soemthing goes wrong when registering the hotplug callback, what
> undoes the MSR setup which might have happened and what takes care of it on
> cpus coming online later? Exactly nothing. Then you have a non-consistent
> behaviour.
> 
> Make stuff symmmetric and correct and not optimized for the sunshine case.

I see.

Just want to make sure I understand it correctly:

sysfs_create_group() should not be called if cpuhp_setup_state() has
 error.

Otherwise, the sysadmin can change the MSR through the sysfs interface.
After that, a CPU is online and its MSR is not updated because cpu_online
is not installed. Then this online CPU has different MSR value from
the other CPUs.

Is that right?

Thanks.

-Fenghua

Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable

[Thomas Gleixner]

On Fri, 9 Aug 2019, Fenghua Yu wrote:
> On Fri, Aug 09, 2019 at 11:49:49AM +0200, Thomas Gleixner wrote:
> > Groan. If soemthing goes wrong when registering the hotplug callback, what
> > undoes the MSR setup which might have happened and what takes care of it on
> > cpus coming online later? Exactly nothing. Then you have a non-consistent
> > behaviour.
> > 
> > Make stuff symmmetric and correct and not optimized for the sunshine case.
> 
> I see.
> 
> Just want to make sure I understand it correctly:
> 
> sysfs_create_group() should not be called if cpuhp_setup_state() has
>  error.
> 
> Otherwise, the sysadmin can change the MSR through the sysfs interface.
> After that, a CPU is online and its MSR is not updated because cpu_online
> is not installed. Then this online CPU has different MSR value from
> the other CPUs.
> 
> Is that right?

Yes. You need to enforce safe and consistent behaviour.

Thanks,

	tglx