[PATCH] ACPI / sysfs: Restructure get_status

[PATCH] ACPI / sysfs: Restructure get_status

[Nathan Chancellor]

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))
-		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,
-- 
2.21.0

Re: [PATCH] ACPI / sysfs: Restructure get_status

[Nick Desaulniers]

On Thu, Mar 7, 2019 at 9:38 AM Nathan Chancellor
<natechancellor@gmail.com> 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);

I'm surprised this does not trip -Wdeclaration-after-statement.

>                 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))
> -               result = acpi_get_event_status(index - num_gpes, status);
> +               return acpi_get_event_status(index - num_gpes, status);
>
> -       return result;
> +       return -EINVAL;
>  }


LGTM, thanks Nathan!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] ACPI / sysfs: Restructure get_status

[Rafael J. Wysocki]

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.

---
 drivers/acpi/sysfs.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -648,26 +648,27 @@ 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) {
-			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
-					"Invalid GPE 0x%x", index));
-			return result;
-		}
-		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);
+	if (index >= num_gpes)
+		return acpi_get_event_status(index - num_gpes, ret);
 
-	return 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 -ENXIO;
+	}
+	status = acpi_get_gpe_status(*handle, index, ret);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
 }
 
 static ssize_t counter_show(struct kobject *kobj,

Re: [PATCH] ACPI / sysfs: Restructure get_status

[Rafael J. Wysocki]

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(-)

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,

Re: [PATCH] ACPI / sysfs: Restructure get_status

[Nathan Chancellor]

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!