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

[Thomas Gleixner]

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