linux-kernel - Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode

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: <2fdc2487-aaae-aa3c-9aa6-0c2bf2379e5a@linux.intel.com>
Date: Thu, 28 Aug 2025 14:10:44 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Daniel <dany97@...e.ca>
cc: matan@...alib.org, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in
 fan_mode_store

On Fri, 22 Aug 2025, Daniel wrote:

> On my LG Gram 16Z95P-K.AA75A8 (2022), writes to
> /sys/devices/platform/lg-laptop/fan_mode have no effect and reads always
> report a status of 0.
> 
> Disassembling the relevant ACPI tables reveals that in the call
> 
> 	lg_wmab(dev, WM_FAN_MODE, WM_SET, x)
> 
> the new mode is read from either the upper nibble or the lower nibble of x,
> depending on the value of some other EC register.  Crucially, when WMAB
> is called twice (once with the fan mode in the upper nibble, once with
> it in the lower nibble), the result of the second call can overwrite
> the first call.
> 
> Fix this by calling WMAB once, with the fan mode set in both nibbles.
> As a bonus, the driver now supports the "Performance" mode seen in
> the Windows LG Control Center app (less aggressive CPU throttling, but
> louder fan noise and shorter battery life).  I can confirm that with
> this patch writing/reading the fan mode works as expected on my laptop,
> although I haven't tested it on any other LG laptops.
> 
> Also, correct the documentation to reflect that a value of 0 corresponds
> to the default mode (what the LG app calls "Optimal") and a value of 1
> corresponds to the silent mode.
> 
> Tested-by: Daniel <dany97@...e.ca>
> Signed-off-by: Daniel <dany97@...e.ca>
> ---
>  .../admin-guide/laptops/lg-laptop.rst         |  4 ++--
>  drivers/platform/x86/lg-laptop.c              | 22 +++++--------------
>  2 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/admin-guide/laptops/lg-laptop.rst b/Documentation/admin-guide/laptops/lg-laptop.rst
> index 67fd6932c..c4dd534f9 100644
> --- a/Documentation/admin-guide/laptops/lg-laptop.rst
> +++ b/Documentation/admin-guide/laptops/lg-laptop.rst
> @@ -48,8 +48,8 @@ This value is reset to 100 when the kernel boots.
>  Fan mode
>  --------
>  
> -Writing 1/0 to /sys/devices/platform/lg-laptop/fan_mode disables/enables
> -the fan silent mode.
> +Writing 0/1/2 to /sys/devices/platform/lg-laptop/fan_mode sets fan mode to
> +Optimal/Silent/Performance respectively.
>  
>  
>  USB charge
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 4b57102c7..b8de6e568 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -274,29 +274,19 @@ static ssize_t fan_mode_store(struct device *dev,
>  			      struct device_attribute *attr,
>  			      const char *buffer, size_t count)
>  {
> -	bool value;
> +	unsigned long value;
>  	union acpi_object *r;
> -	u32 m;
>  	int ret;
>  
> -	ret = kstrtobool(buffer, &value);
> +	ret = kstrtoul(buffer, 10, &value);
>  	if (ret)
>  		return ret;
> +	if (value >= 3)
> +		return -EINVAL;
>  
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_GET, 0);
> +	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (value << 4) | value);

Is it okay to remove preserving the other bits?

Please name these field with defined GENMASK() and then use FIELD_PREP() 
here for both fields.

>  	if (!r)
>  		return -EIO;
> -
> -	if (r->type != ACPI_TYPE_INTEGER) {
> -		kfree(r);
> -		return -EIO;
> -	}
> -
> -	m = r->integer.value;
> -	kfree(r);
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (m & 0xffffff0f) | (value << 4));
> -	kfree(r);
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (m & 0xfffffff0) | value);
>  	kfree(r);
>  
>  	return count;
> @@ -317,7 +307,7 @@ static ssize_t fan_mode_show(struct device *dev,
>  		return -EIO;
>  	}
>  
> -	status = r->integer.value & 0x01;
> +	status = r->integer.value & 0x03;

This looks also like a field so should be named with a define?

>  	kfree(r);
>  
>  	return sysfs_emit(buffer, "%d\n", status);
> 

-- 
 i.