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

[Mario Limonciello <mario.limonciello@amd.com>]

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