Regression in asus-wmi due to fan curve patches

Regression in asus-wmi due to fan curve patches

[Abhijeet Viswa]

Hi,

Firstly, apologies if I have included/excluded the wrong mailing list or 
persons in this email. This is my first time doing this and I've tried 
my best to make sure it is accurate.

I am facing a regression in the mainline of the kernel (commit 
0457e5153e0e8420134f60921349099e907264ca) with the asus-wmi platform 
driver. The driver fails to load with the following dmesg:

	asus-nb-wmi: probe of asus-nb-wmi failed with error -61

I have an ASUS TUF FX506 laptop.

I traced the regression to the method fan_curve_get_factory_default. It 
calls a WMI method which is expected to return a data buffer. However, 
if the device does not support fan curve method it is supposed to return 
the integer error code ASUS_WMI_UNSUPPORTED_METHOD.

However, my laptop returns a value 0 to indicate that the method is not 
supported:

                 If ((IIA0 == 0x00110024))
                 {
                     Return (Zero)
                 }

                 If ((IIA0 == 0x00110025))
                 {
                     Return (Zero)
                 }

This means that on lines 395-407 in the method 
asus_wmi_evaluate_method_buf, the if condition err == 0 evaluates to 
true an -ENODATA (-61) is returned.

         case ACPI_TYPE_INTEGER:
                 err = (u32)obj->integer.value;

                 if (err == ASUS_WMI_UNSUPPORTED_METHOD)
                         err = -ENODEV;
                 /*
                  * At least one method returns a 0 with no buffer if no arg
                  * is provided, such as ASUS_WMI_DEVID_CPU_FAN_CURVE
                  */
                 if (err == 0)
                         err = -ENODATA;
                 break;

I am not sure the extent of ASUS laptops that are affected. TUF series 
laptops do not support fan curve control and so I presume many of them 
are affected by this regression.

I have tested a patch which selectively ignores the -ENODATA error code 
when probing for fan curve control. However, I'm not sure if this is the 
right way to do things and hence have no included the patch here.

Once again, I apologize if this email is not how things are normally 
done and would love to hear feedback on the same.

Thanks
Abhijeet

Re: Regression in asus-wmi due to fan curve patches

[Hans de Goede]

Hi,

On 2/5/22 11:10, Abhijeet Viswa wrote:
> Hi,
> 
> Firstly, apologies if I have included/excluded the wrong mailing list or persons in this email. This is my first time doing this and I've tried my best to make sure it is accurate.

No worries, it looks like you've done a pretty good job at picking the
right people + lists. And even if you didn't with regressions like this
*the* most important thing is to get the word out quickly, so thank
you for doing that!

> I am facing a regression in the mainline of the kernel (commit 0457e5153e0e8420134f60921349099e907264ca) with the asus-wmi platform driver. The driver fails to load with the following dmesg:
> 
>     asus-nb-wmi: probe of asus-nb-wmi failed with error -61
> 
> I have an ASUS TUF FX506 laptop.
> 
> I traced the regression to the method fan_curve_get_factory_default. It calls a WMI method which is expected to return a data buffer. However, if the device does not support fan curve method it is supposed to return the integer error code ASUS_WMI_UNSUPPORTED_METHOD.
> 
> However, my laptop returns a value 0 to indicate that the method is not supported:
> 
>                 If ((IIA0 == 0x00110024))
>                 {
>                     Return (Zero)
>                 }
> 
>                 If ((IIA0 == 0x00110025))
>                 {
>                     Return (Zero)
>                 }
> 
> This means that on lines 395-407 in the method asus_wmi_evaluate_method_buf, the if condition err == 0 evaluates to true an -ENODATA (-61) is returned.
> 
>         case ACPI_TYPE_INTEGER:
>                 err = (u32)obj->integer.value;
> 
>                 if (err == ASUS_WMI_UNSUPPORTED_METHOD)
>                         err = -ENODEV;
>                 /*
>                  * At least one method returns a 0 with no buffer if no arg
>                  * is provided, such as ASUS_WMI_DEVID_CPU_FAN_CURVE
>                  */
>                 if (err == 0)
>                         err = -ENODATA;
>                 break;
> 
> I am not sure the extent of ASUS laptops that are affected. TUF series laptops do not support fan curve control and so I presume many of them are affected by this regression.
> 
> I have tested a patch which selectively ignores the -ENODATA error code when probing for fan curve control. However, I'm not sure if this is the right way to do things and hence have no included the patch here.

That sounds like it is the right way to do it, can you share your patch please ?

And in the case I end up using your patch as is, may I add a:

Signed-off-by: Abhijeet Viswa <abhijeetviswa@gmail.com>

To the patch when committing it?

By adding this line you indicate that you are the author of the code and
are submitting it under the existing license for the file which you are
modifying (typically GPL-2.0) or that you have permission from the author
to submit it under this license. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> Once again, I apologize if this email is not how things are normally done and would love to hear feedback on the same.

Once again, this is the right way, so no worries and thank you for reporting this.

Regards,

Hans

Re: Regression in asus-wmi due to fan curve patches

[Abhijeet Viswa]

Hi, 

On 05/02/22 15:46, Hans de Goede wrote:
>> Firstly, apologies if I have included/excluded the wrong mailing list or persons in this email. This is my first time doing this and I've tried my best to make sure it is accurate.
> 
> No worries, it looks like you've done a pretty good job at picking the
> right people + lists. And even if you didn't with regressions like this
> *the* most important thing is to get the word out quickly, so thank
> you for doing that!

Thanks. I'm happy to hear this.

I tried a git-format patch which created an email patch. Wasn't sure if I had to send a separate patch email, so I've decided to inline the patch contents at the end of this email. And, I certify that the changes are mine and license them under the same license as the files modified.

Hopefully inlining it is fine. If not, I could provide it as an attachment.

Thanks
Abhijeet

~~~

From f5bae0a579dc211c329c7aa08837e462aee1af6b Mon Sep 17 00:00:00 2001
From: Abhijeet V <abhijeetviswa@gmail.com>
Date: Sat, 5 Feb 2022 15:57:23 +0530
Subject: [PATCH] asus-wmi: Fix regression when probing for fan curve control

The fan curve control patches introduced a regression for atleast the
TUF FX506 and possibly other TUF series laptops that do not have support
for fan curve control.

As part of the probing process, asus_wmi_evaluate_method_buf is called
to get the factory default fan curve . The WMI management function
returns 0 on certain laptops to indicate lack of fan curve control
instead of ASUS_WMI_UNSUPPORTED_METHOD. This 0 is transformed to
-ENODATA which results in failure when probing.

Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a3b83b22a3b..adeb58765dc 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3585,7 +3585,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 		goto fail_hwmon;
 
 	err = asus_wmi_custom_fan_curve_init(asus);
-	if (err)
+	if (err && err != -ENODATA)
 		goto fail_custom_fan_curve;
 
 	err = asus_wmi_led_init(asus);
-- 
2.35.1

Re: Regression in asus-wmi due to fan curve patches

[Hans de Goede]

Hi,

On 2/5/22 12:01, Abhijeet Viswa wrote:
> Hi, 
> 
> On 05/02/22 15:46, Hans de Goede wrote:
>>> Firstly, apologies if I have included/excluded the wrong mailing list or persons in this email. This is my first time doing this and I've tried my best to make sure it is accurate.
>>
>> No worries, it looks like you've done a pretty good job at picking the
>> right people + lists. And even if you didn't with regressions like this
>> *the* most important thing is to get the word out quickly, so thank
>> you for doing that!
> 
> Thanks. I'm happy to hear this.
> 
> I tried a git-format patch which created an email patch. Wasn't sure if I had to send a separate patch email, so I've decided to inline the patch contents at the end of this email. And, I certify that the changes are mine and license them under the same license as the files modified.

Thank you for the patch and for the nice commit message.

I've come up with a slightly different version where -ENODATA is checked
inside the fan_curve_check_present() helper. This way if e.g. there
is a CPU fan-curve but no GPU fan-curve, because e.g. there is no
separate GPU fan the hwmon device will still get registered for just
the CPU fan curve.

I'll send out my version right after this email, if you can test it
that would be great.

> Hopefully inlining it is fine. If not, I could provide it as an attachment.

Since I've come up with a different approach I did not try to apply it,
but inlining often leads to issues due to replaceing tabs with spaces
and linewrapping. Although it seems your mail-client has done neither,
which is good.

Still for the next time, the preferred way to submit patches is to
use git send-email which does not have any of these issues.

Regards,

Hans


> ~~~
> 
> From f5bae0a579dc211c329c7aa08837e462aee1af6b Mon Sep 17 00:00:00 2001
> From: Abhijeet V <abhijeetviswa@gmail.com>
> Date: Sat, 5 Feb 2022 15:57:23 +0530
> Subject: [PATCH] asus-wmi: Fix regression when probing for fan curve control
> 
> The fan curve control patches introduced a regression for atleast the
> TUF FX506 and possibly other TUF series laptops that do not have support
> for fan curve control.
> 
> As part of the probing process, asus_wmi_evaluate_method_buf is called
> to get the factory default fan curve . The WMI management function
> returns 0 on certain laptops to indicate lack of fan curve control
> instead of ASUS_WMI_UNSUPPORTED_METHOD. This 0 is transformed to
> -ENODATA which results in failure when probing.
> 
> Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>
> ---
>  drivers/platform/x86/asus-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index a3b83b22a3b..adeb58765dc 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3585,7 +3585,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  		goto fail_hwmon;
>  
>  	err = asus_wmi_custom_fan_curve_init(asus);
> -	if (err)
> +	if (err && err != -ENODATA)
>  		goto fail_custom_fan_curve;
>  
>  	err = asus_wmi_led_init(asus);
>