Re: [PATCH] asus-laptop: Fix an uninitialized variable

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

On Wed, 26 Mar 2025, Arefev wrote:

> 25.03.2025 14:18, Ilpo Järvinen пишет:
> > On Tue, 25 Mar 2025, Denis Arefev wrote:
> > 
> > > The value returned by the acpi_evaluate_integer() function is not
> > > checked, but the result is not always successful, so an uninitialized
> > > 'val' variable may be used in calculations.
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: b23910c2194e ("asus-laptop: Pegatron Lucid accelerometer")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Denis Arefev <arefev@swemel.ru>
> > > ---
> > >   drivers/platform/x86/asus-laptop.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/asus-laptop.c
> > > b/drivers/platform/x86/asus-laptop.c
> > > index d460dd194f19..b74b7d0eb6c2 100644
> > > --- a/drivers/platform/x86/asus-laptop.c
> > > +++ b/drivers/platform/x86/asus-laptop.c
> > > @@ -427,7 +427,7 @@ static int asus_pega_lucid_set(struct asus_laptop
> > > *asus, int unit, bool enable)
> > >   static int pega_acc_axis(struct asus_laptop *asus, int curr, char
> > > *method)
> > >   {
> > >   	int i, delta;
> > > -	unsigned long long val;
> > > +	unsigned long long val = PEGA_ACC_CLAMP;
> > >   	for (i = 0; i < PEGA_ACC_RETRIES; i++) {
> > >   		acpi_evaluate_integer(asus->handle, method, NULL, &val);
> > Shouldn't you handle the error from acpi_evaluate_integer() properly
> > instead?
> > 
> Apparently, the developer realized that the output is very noisy and
> therefore created an algorithm that will surely return a good result.
> 
> I did not check the return value, because if acpi_evaluate_integer()
> cannot read the values of accelerometers, 'val' will remain
> uninitialized and will be used in further calculations.

But if ACPI doesn't provide a value, why should that clamp value be used 
instead? I'd tend to think curr would be more suitable "default".

But shouldn't the loop either use continue to retry or the function 
return curr right away if acpi_evaluate_integer() returns an error?

I just don't see how your patch improves situation here despite silencing 
the checker tool.

-- 
 i.