* [PATCH] ACPI / sysfs: Restructure get_status
@ 2019-03-07 17:38 Nathan Chancellor
2019-03-07 18:21 ` Nick Desaulniers
2019-03-11 22:46 ` Rafael J. Wysocki
0 siblings, 2 replies; 5+ messages in thread
From: Nathan Chancellor @ 2019-03-07 17:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, clang-built-linux, Nick Desaulniers,
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI / sysfs: Restructure get_status
2019-03-07 17:38 [PATCH] ACPI / sysfs: Restructure get_status Nathan Chancellor
@ 2019-03-07 18:21 ` Nick Desaulniers
2019-03-11 22:46 ` Rafael J. Wysocki
1 sibling, 0 replies; 5+ messages in thread
From: Nick Desaulniers @ 2019-03-07 18:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, LKML, clang-built-linux
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI / sysfs: Restructure get_status
2019-03-07 17:38 [PATCH] ACPI / sysfs: Restructure get_status Nathan Chancellor
2019-03-07 18:21 ` Nick Desaulniers
@ 2019-03-11 22:46 ` Rafael J. Wysocki
2019-03-11 22:56 ` Rafael J. Wysocki
1 sibling, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-03-11 22:46 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Len Brown, linux-acpi, linux-kernel, clang-built-linux, Nick Desaulniers
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI / sysfs: Restructure get_status
2019-03-11 22:46 ` Rafael J. Wysocki
@ 2019-03-11 22:56 ` Rafael J. Wysocki
2019-03-12 0:07 ` Nathan Chancellor
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-03-11 22:56 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Len Brown, linux-acpi, linux-kernel, clang-built-linux, Nick Desaulniers
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI / sysfs: Restructure get_status
2019-03-11 22:56 ` Rafael J. Wysocki
@ 2019-03-12 0:07 ` Nathan Chancellor
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Chancellor @ 2019-03-12 0:07 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, linux-acpi, linux-kernel, clang-built-linux, Nick Desaulniers
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-12 0:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 17:38 [PATCH] ACPI / sysfs: Restructure get_status Nathan Chancellor
2019-03-07 18:21 ` Nick Desaulniers
2019-03-11 22:46 ` Rafael J. Wysocki
2019-03-11 22:56 ` Rafael J. Wysocki
2019-03-12 0:07 ` Nathan Chancellor
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.