Patchwork ACPI / sysfs: Restructure get_status

login
register
mail settings
Submitter Rafael J. Wysocki
Date March 11, 2019, 10:56 p.m.
Message ID <3133462.vhohJv2aGS@aspire.rjw.lan>
Download mbox | patch
Permalink /patch/746561/
State New
Headers show

Comments

Rafael J. Wysocki - March 11, 2019, 10:56 p.m.
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.
> 
> > -		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(-)
Nathan Chancellor - March 12, 2019, 12:07 a.m.
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!

Patch

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,