linux-kernel - Re: [PATCH] platform/x86: Switch to guard(mutex)

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] [day] [month] [year] [list]

Message-ID: <5e94a03f-193b-895f-0ba3-712240908db0@linux.intel.com>
Date: Tue, 23 Dec 2025 11:44:05 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: chen zhang <chenzhang@...inos.cn>
cc: hdegoede@...hat.com, Hans de Goede <hansg@...nel.org>, frank@...eidel.de, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    chenzhang_0901@....com
Subject: Re: [PATCH] platform/x86: Switch to guard(mutex)

On Mon, 15 Dec 2025, chen zhang wrote:

You're using too generic prefix from the shortlog (in the Subject).

> Instead of using the 'goto label; mutex_unlock()' pattern use
> 'guard(mutex)' which will release the mutex when it goes out of scope.
> 
> Signed-off-by: chen zhang <chenzhang@...inos.cn>
> ---
>  drivers/platform/x86/hdaps.c | 37 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/x86/hdaps.c b/drivers/platform/x86/hdaps.c
> index f11f726d2062..2a11bcaa52f2 100644
> --- a/drivers/platform/x86/hdaps.c
> +++ b/drivers/platform/x86/hdaps.c
> @@ -147,18 +147,16 @@ static int hdaps_readb_one(unsigned int port, u8 *val)
>  {
>  	int ret;
>  
> -	mutex_lock(&hdaps_mtx);
> +	guard(mutex)(&hdaps_mtx);
>  
>  	/* do a sync refresh -- we need to be sure that we read fresh data */
>  	ret = __device_refresh_sync();
>  	if (ret)
> -		goto out;
> +		return ret;
>  
>  	*val = inb(port);
>  	__device_complete();
>  
> -out:
> -	mutex_unlock(&hdaps_mtx);
>  	return ret;

return 0;

>  }
>  
> @@ -208,12 +206,12 @@ static int hdaps_device_init(void)
>  {
>  	int total, ret = -ENXIO;
>  
> -	mutex_lock(&hdaps_mtx);
> +	guard(mutex)(&hdaps_mtx);
>  
>  	outb(0x13, 0x1610);
>  	outb(0x01, 0x161f);
>  	if (__wait_latch(0x161f, 0x00))
> -		goto out;
> +		return ret;

This is not an acceptable conversion. You clearly no longer need ret 
variable to carry the information here but can just write:

		return -ENXIO;

Please do realize if you make this kind of conversion changes to old code 
which are on borderline whether they're more noise and burden (to 
maintainers and reviewers) than useful, you should know all these 
conventions so that no review cycles is wasted on trivial style issues 
like this.

I know some subsystems do push back on patches like this. I find them 
still somewhat useful but you should think the patch through before 
sending, which you clearly didn't here. Cleaning up/refactoring is not 
"easy" unless you do it the wrong way which is to offload thinking to the 
reviewers/maintainers.

>  	/*
>  	 * Most ThinkPads return 0x01.
> @@ -226,7 +224,7 @@ static int hdaps_device_init(void)
>  	if (__check_latch(0x1611, 0x03) &&
>  		     __check_latch(0x1611, 0x02) &&
>  		     __check_latch(0x1611, 0x01))
> -		goto out;
> +		return ret;
>  
>  	printk(KERN_DEBUG "hdaps: initial latch check good (0x%02x)\n",
>  	       __get_latch(0x1611));
> @@ -235,29 +233,29 @@ static int hdaps_device_init(void)
>  	outb(0x81, 0x1611);
>  	outb(0x01, 0x161f);
>  	if (__wait_latch(0x161f, 0x00))
> -		goto out;
> +		return ret;
>  	if (__wait_latch(0x1611, 0x00))
> -		goto out;
> +		return ret;
>  	if (__wait_latch(0x1612, 0x60))
> -		goto out;
> +		return ret;
>  	if (__wait_latch(0x1613, 0x00))
> -		goto out;
> +		return ret;
>  	outb(0x14, 0x1610);
>  	outb(0x01, 0x1611);
>  	outb(0x01, 0x161f);
>  	if (__wait_latch(0x161f, 0x00))
> -		goto out;
> +		return ret;
>  	outb(0x10, 0x1610);
>  	outb(0xc8, 0x1611);
>  	outb(0x00, 0x1612);
>  	outb(0x02, 0x1613);
>  	outb(0x01, 0x161f);
>  	if (__wait_latch(0x161f, 0x00))
> -		goto out;
> +		return ret;
>  	if (__device_refresh_sync())
> -		goto out;
> +		return ret;
>  	if (__wait_latch(0x1611, 0x00))
> -		goto out;
> +		return ret;
>  
>  	/* we have done our dance, now let's wait for the applause */
>  	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
> @@ -273,8 +271,6 @@ static int hdaps_device_init(void)
>  		msleep(INIT_WAIT_MSECS);
>  	}
>  
> -out:
> -	mutex_unlock(&hdaps_mtx);
>  	return ret;
>  }
>  
> @@ -322,17 +318,14 @@ static void hdaps_mousedev_poll(struct input_dev *input_dev)
>  {
>  	int x, y;
>  
> -	mutex_lock(&hdaps_mtx);
> +	guard(mutex)(&hdaps_mtx);
>  
>  	if (__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y))
> -		goto out;
> +		return;
>  
>  	input_report_abs(input_dev, ABS_X, x - rest_x);
>  	input_report_abs(input_dev, ABS_Y, y - rest_y);
>  	input_sync(input_dev);
> -
> -out:
> -	mutex_unlock(&hdaps_mtx);
>  }
>  
>  
> 

-- 
 i.