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

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] [day] [month] [year] [list]

Date: Mon, 19 Feb 2024 09:24:04 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>,
 Hans de Goede <hdegoede@...hat.com>
Cc: "platform-driver-x86@...r.kernel.org"
 <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Only update profile if
 successfully converted

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@....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@...ebb.ca>
> 
> Mark