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

[Thomas Gleixner]

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@xxxxxx>
> 
> 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