Received-SPF: pass (google.com: domain of linux-kernel+bounces-71567-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: ibe194615:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-144-ge5821d614e-fm-20240125.002-ge5821d61
Precedence: bulk
MIME-Version: 1.0
Message-Id: <b37b5e6f-c719-4ff2-b16d-96716e681ca9@app.fastmail.com>
In-Reply-To: <20240217022311.113879-1-mario.limonciello@amd.com>
References: <20240217022311.113879-1-mario.limonciello@amd.com>
Date: Mon, 19 Feb 2024 10:13:54 -0500
From: "Mark Pearson" <mpearson-lenovo@squebb.ca>
To: "Limonciello, Mario" <mario.limonciello@amd.com>,
 "Hans de Goede" <hdegoede@redhat.com>
Cc: 
 "platform-driver-x86@vger.kernel.org" <platform-driver-x86@vger.kernel.org>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Only update profile if successfully
 converted
Content-Type: text/plain

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