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@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.
> >
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@xxxxxxxxx>
Thank you for the reply and patch!
