[PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[Daniel]

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@live.ca>
Signed-off-by: Daniel <dany97@live.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);
 	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;
 	kfree(r);
 
 	return sysfs_emit(buffer, "%d\n", status);
-- 
2.50.1

Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[Ilpo Järvinen]

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@live.ca>
> Signed-off-by: Daniel <dany97@live.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.

Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[Ilpo Järvinen]

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

On Thu, 28 Aug 2025, Ilpo Järvinen wrote:

> 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.

Oh, one more thing. I think this would warrant a Fixes tag pointing to the 
original commit.

--
 i.

> > 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@live.ca>
> > Signed-off-by: Daniel <dany97@live.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);
> > 
> 
> 

Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[Daniel]

> Is it okay to remove preserving the other bits?

The WMAB call reads only the lower nibble or the upper nibble from the
lowest byte.  Similarly when the fan status is read from WMAB, something
of the form ((x << 4) | x) is always returned.

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

I don't see why something like that should be necessary, seems overkill,
but if you insist then I'll do it.

I'll try to send another patch sometime soon, I've been very busy.

Re: [PATCH] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store

[Ilpo Järvinen]

On Sat, 6 Sep 2025, Daniel wrote:

> > Is it okay to remove preserving the other bits?
> 
> The WMAB call reads only the lower nibble or the upper nibble from the
> lowest byte.  Similarly when the fan status is read from WMAB, something
> of the form ((x << 4) | x) is always returned.

So the upper portion of the mask in the old code was overkill? Please note 
that down into the changelog so it's recorded into the git history. This 
avoids somebody having to wonder why a seemingly unrelated change like 
that was made if somebody looks at this commit after a few years from the 
git history.

> > Please name these field with defined GENMASK() and then use FIELD_PREP()
> > here for both fields.
> 
> I don't see why something like that should be necessary, seems overkill,
> but if you insist then I'll do it.

Yes, I insist. When we touch older code, the expectation is to use modern 
style and helpers. In here, naming the fields will improve readability 
and document the fields more explicitly.

> I'll try to send another patch sometime soon, I've been very busy.

Don't worry, there's time. :-)

-- 
 i.