linux-kernel - Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8a106cfe-f7cd-4660-983a-feba627cdcab@redhat.com>
Date: Wed, 4 Sep 2024 20:14:53 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Gergo Koteles <soyer@....hu>, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: Ike Panhc <ike.pan@...onical.com>, Peter Zijlstra <peterz@...radead.org>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the
 scope_guard() clear of its scope

Hi,

On 8/29/24 6:50 PM, Andy Shevchenko wrote:
> First of all, it's a bit counterintuitive to have something like
> 
> 	int err;
> 	...
> 	scoped_guard(...)
> 		err = foo(...);
> 	if (err)
> 		return err;
> 
> Second, with a particular kernel configuration and compiler version in
> one of such cases the objtool is not happy:
> 
>   ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section
> 
> I'm not an expert on all this, but the theory is that compiler and
> linker in this case can't understand that 'result' variable will be
> always initialized as long as no error has been returned. Assigning
> 'result' to a dummy value helps with this. Note, that fixing the
> scoped_guard() scope (as per above) does not make issue gone.
> 
> That said, assign dummy value and make the scope_guard() clear of its scope.
> For the sake of consistency do it in the entire file.
> 
> Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands")
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
> 
> This has been Cc'ed to objtool and clang maintainers to have a look and
> tell if this can be addressed in a better way.
> 
>  drivers/platform/x86/ideapad-laptop.c | 48 +++++++++++++++------------
>  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 35c75bcff195..c64dfc56651d 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -554,13 +554,14 @@ static ssize_t camera_power_show(struct device *dev,
>  				 char *buf)
>  {
>  	struct ideapad_private *priv = dev_get_drvdata(dev);
> -	unsigned long result;
> +	unsigned long result = 0;
>  	int err;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	return sysfs_emit(buf, "%d\n", !!result);
>  }
> @@ -577,10 +578,11 @@ static ssize_t camera_power_store(struct device *dev,
>  	if (err)
>  		return err;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	return count;
>  }
> @@ -628,13 +630,14 @@ static ssize_t fan_mode_show(struct device *dev,
>  			     char *buf)
>  {
>  	struct ideapad_private *priv = dev_get_drvdata(dev);
> -	unsigned long result;
> +	unsigned long result = 0;
>  	int err;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	return sysfs_emit(buf, "%lu\n", result);
>  }
> @@ -654,10 +657,11 @@ static ssize_t fan_mode_store(struct device *dev,
>  	if (state > 4 || state == 3)
>  		return -EINVAL;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	return count;
>  }
> @@ -737,13 +741,14 @@ static ssize_t touchpad_show(struct device *dev,
>  			     char *buf)
>  {
>  	struct ideapad_private *priv = dev_get_drvdata(dev);
> -	unsigned long result;
> +	unsigned long result = 0;
>  	int err;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	priv->r_touchpad_val = result;
>  
> @@ -762,10 +767,11 @@ static ssize_t touchpad_store(struct device *dev,
>  	if (err)
>  		return err;
>  
> -	scoped_guard(mutex, &priv->vpc_mutex)
> +	scoped_guard(mutex, &priv->vpc_mutex) {
>  		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	priv->r_touchpad_val = state;
>