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@xxxxxxxxxx>
> > Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx>
> > ---
> > 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,
