[PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully converted

[PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully converted

[Mario Limonciello]

Randomly a Lenovo Z13 will trigger a kernel warning traceback from this
condition:

```
if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
```

This happens because thinkpad-acpi always assumes that
convert_dytc_to_profile() successfully updated the profile. On the
contrary a condition can occur that when dytc_profile_refresh() is called
the profile doesn't get updated as there is a -EOPNOTSUPP branch.

Catch this situation and avoid updating the profile. Also log this into
dynamic debugging in case any other modes should be added in the future.

Fixes: c3bfcd4c6762 ("platform/x86: thinkpad_acpi: Add platform profile support")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
BTW - This isn't new.  I've been seeing this a long time, but I just finally
got annoyed enough by it to find the code that triggered the sequence.

 drivers/platform/x86/thinkpad_acpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c4895e9bc714..5ecd9d33250d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10308,6 +10308,7 @@ static int convert_dytc_to_profile(int funcmode, int dytcmode,
 		return 0;
 	default:
 		/* Unknown function */
+		pr_debug("unknown function 0x%x\n", funcmode);
 		return -EOPNOTSUPP;
 	}
 	return 0;
@@ -10493,8 +10494,8 @@ static void dytc_profile_refresh(void)
 		return;
 
 	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
-	convert_dytc_to_profile(funcmode, perfmode, &profile);
-	if (profile != dytc_current_profile) {
+	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
+	if (!err && profile != dytc_current_profile) {
 		dytc_current_profile = profile;
 		platform_profile_notify();
 	}
-- 
2.34.1

Re: [PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully converted

[Hans de Goede]

Hi,

On 2/17/24 03:23, Mario Limonciello wrote:
> Randomly a Lenovo Z13 will trigger a kernel warning traceback from this
> condition:
> 
> ```
> if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> ```
> 
> This happens because thinkpad-acpi always assumes that
> convert_dytc_to_profile() successfully updated the profile. On the
> contrary a condition can occur that when dytc_profile_refresh() is called
> the profile doesn't get updated as there is a -EOPNOTSUPP branch.
> 
> Catch this situation and avoid updating the profile. Also log this into
> dynamic debugging in case any other modes should be added in the future.
> 
> Fixes: c3bfcd4c6762 ("platform/x86: thinkpad_acpi: Add platform profile support")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.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.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans




> ---
> BTW - This isn't new.  I've been seeing this a long time, but I just finally
> got annoyed enough by it to find the code that triggered the sequence.
> 
>  drivers/platform/x86/thinkpad_acpi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c4895e9bc714..5ecd9d33250d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10308,6 +10308,7 @@ static int convert_dytc_to_profile(int funcmode, int dytcmode,
>  		return 0;
>  	default:
>  		/* Unknown function */
> +		pr_debug("unknown function 0x%x\n", funcmode);
>  		return -EOPNOTSUPP;
>  	}
>  	return 0;
> @@ -10493,8 +10494,8 @@ static void dytc_profile_refresh(void)
>  		return;
>  
>  	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> -	convert_dytc_to_profile(funcmode, perfmode, &profile);
> -	if (profile != dytc_current_profile) {
> +	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
> +	if (!err && profile != dytc_current_profile) {
>  		dytc_current_profile = profile;
>  		platform_profile_notify();
>  	}

Re: [PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully converted

[Mark Pearson]

Hi Mario

On Fri, Feb 16, 2024, at 9:23 PM, Mario Limonciello wrote:
> Randomly a Lenovo Z13 will trigger a kernel warning traceback from this
> condition:
>
> ```
> if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> ```
>
> This happens because thinkpad-acpi always assumes that
> convert_dytc_to_profile() successfully updated the profile. On the
> contrary a condition can occur that when dytc_profile_refresh() is called
> the profile doesn't get updated as there is a -EOPNOTSUPP branch.
>
> Catch this situation and avoid updating the profile. Also log this into
> dynamic debugging in case any other modes should be added in the future.
>
> Fixes: c3bfcd4c6762 ("platform/x86: thinkpad_acpi: Add platform profile 
> support")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> BTW - This isn't new.  I've been seeing this a long time, but I just 
> finally
> got annoyed enough by it to find the code that triggered the sequence.

I've never seen this on my systems - is there anything in particular that can be used to reproduce the issue? I'll follow up with the FW team as setting the profile shouldn't (to my knowledge) fail. Agreed it should be handled if it does fail though.

>
>  drivers/platform/x86/thinkpad_acpi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index c4895e9bc714..5ecd9d33250d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10308,6 +10308,7 @@ static int convert_dytc_to_profile(int 
> funcmode, int dytcmode,
>  		return 0;
>  	default:
>  		/* Unknown function */
> +		pr_debug("unknown function 0x%x\n", funcmode);
>  		return -EOPNOTSUPP;
>  	}
>  	return 0;
> @@ -10493,8 +10494,8 @@ static void dytc_profile_refresh(void)
>  		return;
> 
>  	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> -	convert_dytc_to_profile(funcmode, perfmode, &profile);
> -	if (profile != dytc_current_profile) {
> +	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
> +	if (!err && profile != dytc_current_profile) {
>  		dytc_current_profile = profile;
>  		platform_profile_notify();
>  	}
> -- 
> 2.34.1

Looks good to me. Thank you!
Reviewed-by Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

Re: [PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully converted

[Mario Limonciello]

On 2/19/2024 09:13, Mark Pearson wrote:
> Hi Mario
> 
> On Fri, Feb 16, 2024, at 9:23 PM, Mario Limonciello wrote:
>> Randomly a Lenovo Z13 will trigger a kernel warning traceback from this
>> condition:
>>
>> ```
>> if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
>> ```
>>
>> This happens because thinkpad-acpi always assumes that
>> convert_dytc_to_profile() successfully updated the profile. On the
>> contrary a condition can occur that when dytc_profile_refresh() is called
>> the profile doesn't get updated as there is a -EOPNOTSUPP branch.
>>
>> Catch this situation and avoid updating the profile. Also log this into
>> dynamic debugging in case any other modes should be added in the future.
>>
>> Fixes: c3bfcd4c6762 ("platform/x86: thinkpad_acpi: Add platform profile
>> support")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> BTW - This isn't new.  I've been seeing this a long time, but I just
>> finally
>> got annoyed enough by it to find the code that triggered the sequence.
> 
> I've never seen this on my systems - is there anything in particular that can be used to reproduce the issue? I'll follow up with the FW team as setting the profile shouldn't (to my knowledge) fail. Agreed it should be handled if it does fail though.

I think it's mostly at bootup I've seen it.

With dynamic debug turned on I can see the "Unknown function" prints 0x0.

> 
>>
>>   drivers/platform/x86/thinkpad_acpi.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index c4895e9bc714..5ecd9d33250d 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10308,6 +10308,7 @@ static int convert_dytc_to_profile(int
>> funcmode, int dytcmode,
>>   		return 0;
>>   	default:
>>   		/* Unknown function */
>> +		pr_debug("unknown function 0x%x\n", funcmode);
>>   		return -EOPNOTSUPP;
>>   	}
>>   	return 0;
>> @@ -10493,8 +10494,8 @@ static void dytc_profile_refresh(void)
>>   		return;
>>
>>   	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>> -	convert_dytc_to_profile(funcmode, perfmode, &profile);
>> -	if (profile != dytc_current_profile) {
>> +	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
>> +	if (!err && profile != dytc_current_profile) {
>>   		dytc_current_profile = profile;
>>   		platform_profile_notify();
>>   	}
>> -- 
>> 2.34.1
> 
> Looks good to me. Thank you!
> Reviewed-by Mark Pearson <mpearson-lenovo@squebb.ca>
> 
> Mark