Re: [PATCH] ACPI / sysfs: Restructure get_status

[Nathan Chancellor <natechancellor@gmail.com>]

On Mon, Mar 11, 2019 at 11:56:27PM +0100, Rafael J. Wysocki wrote:
> On Monday, March 11, 2019 11:46:14 PM CET Rafael J. Wysocki wrote:
> > On Thursday, March 7, 2019 6:38:02 PM CET Nathan Chancellor wrote:
> > > When building with -Wsometimes-uninitialized, Clang warns:
> > > 
> > > drivers/acpi/sysfs.c:667:13: warning: variable 'result' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> > > 
> > > Clang can't determine that all cases are covered by the two separate if
> > > statements. We could combine then to look like this:
> > > 
> > >     int result;
> > > 
> > >     if (...) {
> > >         ...
> > >     } else if {
> > >         ...
> > >     } else {
> > >         result -EINVAL;
> > >     }
> > > 
> > >     return result;
> > > 
> > > However, at that point, we can further simplify this function by only
> > > using result when absolutely needed and just direct returning the value
> > > of the function.
> > > 
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/388
> > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/acpi/sysfs.c | 13 ++++---------
> > >  1 file changed, 4 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> > > index 41324f0b1bee..6ed785cadad9 100644
> > > --- a/drivers/acpi/sysfs.c
> > > +++ b/drivers/acpi/sysfs.c
> > > @@ -651,23 +651,18 @@ static void acpi_global_event_handler(u32 event_type, acpi_handle device,
> > >  static int get_status(u32 index, acpi_event_status *status,
> > >  		      acpi_handle *handle)
> > >  {
> > > -	int result;
> > > -
> > > -	if (index >= num_gpes + ACPI_NUM_FIXED_EVENTS)
> > > -		return -EINVAL;
> > > -
> > >  	if (index < num_gpes) {
> > > -		result = acpi_get_gpe_device(index, handle);
> > > +		int result = acpi_get_gpe_device(index, handle);
> > >  		if (result) {
> > >  			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
> > >  					"Invalid GPE 0x%x", index));
> > >  			return result;
> > >  		}
> > > -		result = acpi_get_gpe_status(*handle, index, status);
> > > +		return acpi_get_gpe_status(*handle, index, status);
> > >  	} else if (index < (num_gpes + ACPI_NUM_FIXED_EVENTS))
> > 
> > In principle, it would suffice to replace the "else if (...)" with "else"
> > to fix the Clang warning AFAICS, but that's not the only issue with this
> > function.
> > 

Correct, that would be the simplest fix for the warning.

> > > -		result = acpi_get_event_status(index - num_gpes, status);
> > > +		return acpi_get_event_status(index - num_gpes, status);
> > >  
> > > -	return result;
> > > +	return -EINVAL;
> > >  }
> > >  
> > >  static ssize_t counter_show(struct kobject *kobj,
> > 
> > Namely, it confuses errno with acpi_status and may cause the latter to be
> > returned to user space in certain situations which should never be done.
> > 
> > So, I'd prefer to apply something like the (untested so far) patch below.
> 
> Actually, acpi_get_event_status() returns acpi_status too, so something like
> this rather:
> 
> ---
>  drivers/acpi/sysfs.c |   21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sysfs.c
> +++ linux-pm/drivers/acpi/sysfs.c
> @@ -648,26 +648,29 @@ static void acpi_global_event_handler(u3
>  	}
>  }
>  
> -static int get_status(u32 index, acpi_event_status *status,
> +static int get_status(u32 index, acpi_event_status *ret,
>  		      acpi_handle *handle)
>  {
> -	int result;
> +	acpi_status status;
>  
>  	if (index >= num_gpes + ACPI_NUM_FIXED_EVENTS)
>  		return -EINVAL;
>  
>  	if (index < num_gpes) {
> -		result = acpi_get_gpe_device(index, handle);
> -		if (result) {
> +		status = acpi_get_gpe_device(index, handle);
> +		if (ACPI_FAILURE(status)) {
>  			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
>  					"Invalid GPE 0x%x", index));
> -			return result;
> +			return -ENXIO;
>  		}
> -		result = acpi_get_gpe_status(*handle, index, status);
> -	} else if (index < (num_gpes + ACPI_NUM_FIXED_EVENTS))
> -		result = acpi_get_event_status(index - num_gpes, status);
> +		status = acpi_get_gpe_status(*handle, index, ret);
> +	} else {
> +		status = acpi_get_event_status(index - num_gpes, ret);
> +	}
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
>  
> -	return result;
> +	return 0;
>  }
>  
>  static ssize_t counter_show(struct kobject *kobj,
> 

Seems reasonable. There are no warnings from this.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Thank you for the reply and patch!