| Message ID | 20230112204501.487920-1-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Remove AMT enablement | expand |
[Public] > -----Original Message----- > From: Mark Pearson <mpearson-lenovo@squebb.ca> > Sent: Thursday, January 12, 2023 14:45 > To: mpearson-lenovo@squebb.ca > Cc: hdegoede@redhat.com; Limonciello, Mario > <Mario.Limonciello@amd.com>; markgross@kernel.org; platform-driver- > x86@vger.kernel.org > Subject: [PATCH] platform/x86: thinkpad_acpi: Remove AMT enablement > This title really isn't accurate is it? AMT is still there, it's really more of a bug fix for reporting the right profiles when AMT is active (which should be balanced). > Recently AMT mode was enabled (somewhat unexpectedly) on the Lenovo > Z13 platform. The FW is advertising it is available and the driver tries > to use it - unfortunately it reports the profile mode incorrectly. > > Note, there is also some extra work needed to enable the dynamic aspect > of AMT support that I will be following up with; but more testing is > needed first. This patch just fixes things so the profiles are reported > correctly. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> With the title fixed. Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> Suggest this to Cc to 6.1-stable as well as it's fixing a power profile reporting bug. > --- > drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c > b/drivers/platform/x86/thinkpad_acpi.c > index 1195293b22fd..a95946800ae9 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -10311,9 +10311,11 @@ static DEFINE_MUTEX(dytc_mutex); > static int dytc_capabilities; > static bool dytc_mmc_get_available; > > -static int convert_dytc_to_profile(int dytcmode, enum > platform_profile_option *profile) > +static int convert_dytc_to_profile(int funcmode, int dytcmode, > + enum platform_profile_option *profile) > { > - if (dytc_capabilities & BIT(DYTC_FC_MMC)) { > + switch (funcmode) { > + case DYTC_FUNCTION_MMC: > switch (dytcmode) { > case DYTC_MODE_MMC_LOWPOWER: > *profile = PLATFORM_PROFILE_LOW_POWER; > @@ -10329,8 +10331,7 @@ static int convert_dytc_to_profile(int dytcmode, > enum platform_profile_option *p > return -EINVAL; > } > return 0; > - } > - if (dytc_capabilities & BIT(DYTC_FC_PSC)) { > + case DYTC_FUNCTION_PSC: > switch (dytcmode) { > case DYTC_MODE_PSC_LOWPOWER: > *profile = PLATFORM_PROFILE_LOW_POWER; > @@ -10344,6 +10345,14 @@ static int convert_dytc_to_profile(int dytcmode, > enum platform_profile_option *p > default: /* Unknown mode */ > return -EINVAL; > } > + return 0; > + case DYTC_FUNCTION_AMT: > + /* For now return balanced. It's the closest we have to 'auto' > */ > + *profile = PLATFORM_PROFILE_BALANCED; > + return 0; > + default: > + /* Unknown function */ > + return -EOPNOTSUPP; > } > return 0; > } > @@ -10492,6 +10501,7 @@ static int dytc_profile_set(struct > platform_profile_handler *pprof, > err = > dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_PSC, perfmode, > 1), &output); > if (err) > goto unlock; > + > /* system supports AMT, activate it when on balanced */ > if (dytc_capabilities & BIT(DYTC_FC_AMT)) > dytc_control_amt(profile == > PLATFORM_PROFILE_BALANCED); > @@ -10507,7 +10517,7 @@ static void dytc_profile_refresh(void) > { > enum platform_profile_option profile; > int output, err = 0; > - int perfmode; > + int perfmode, funcmode; > > mutex_lock(&dytc_mutex); > if (dytc_capabilities & BIT(DYTC_FC_MMC)) { > @@ -10522,8 +10532,9 @@ static void dytc_profile_refresh(void) > if (err) > return; > > + funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; > perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; > - convert_dytc_to_profile(perfmode, &profile); > + convert_dytc_to_profile(funcmode, perfmode, &profile); > if (profile != dytc_current_profile) { > dytc_current_profile = profile; > platform_profile_notify(); > -- > 2.39.0
On 1/12/2023 15:06, Mark Pearson wrote: > On Thu, Jan 12, 2023, at 3:49 PM, Limonciello, Mario wrote: >> [Public] >> >> >> >> > -----Original Message----- >> > From: Mark Pearson <mpearson-lenovo@squebb.ca >> <mailto:mpearson-lenovo@squebb.ca>> >> > Sent: Thursday, January 12, 2023 14:45 >> > To: mpearson-lenovo@squebb.ca <mailto:mpearson-lenovo@squebb.ca> >> > Cc: hdegoede@redhat.com <mailto:hdegoede@redhat.com>; Limonciello, Mario >> > <Mario.Limonciello@amd.com <mailto:Mario.Limonciello@amd.com>>; >> markgross@kernel.org <mailto:markgross@kernel.org>; platform-driver- >> > x86@vger.kernel.org <mailto:x86@vger.kernel.org> >> > Subject: [PATCH] platform/x86: thinkpad_acpi: Remove AMT enablement >> > >> >> This title really isn't accurate is it? AMT is still there, it's >> really more of a bug >> fix for reporting the right profiles when AMT is active (which should >> be balanced). > Shoot my bad. I did two versions of the patch - one to remove the > feature, and one to fix it. > I did some testing and the results from the fix version were better so I > updated my commit message....and missed the title. I'm an idiot. > > I assume I should resend the patch with the correct title? Probably best to do. > >> > Recently AMT mode was enabled (somewhat unexpectedly) on the Lenovo >> > Z13 platform. The FW is advertising it is available and the driver tries >> > to use it - unfortunately it reports the profile mode incorrectly. >> > >> > Note, there is also some extra work needed to enable the dynamic aspect >> > of AMT support that I will be following up with; but more testing is >> > needed first. This patch just fixes things so the profiles are reported >> > correctly. >> > >> > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca >> <mailto:mpearson-lenovo@squebb.ca>> >> >> With the title fixed. >> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com >> <mailto:mario.limonciello@amd.com>> >> >> Suggest this to Cc to 6.1-stable as well as it's fixing a power >> profile reporting bug. > Ah - I was going to propose it to stable once it was reviewed and > accepted. Is it normal to do that on the first pass? > > Mark You can do it both ways. https://www.kernel.org/doc/html/next/process/stable-kernel-rules.html I was suggesting method 1 since this is a bug fix. When you submit V2, some other things you should add (besides my tag and fix the title): Reported-by: <person's email if you have one> Link: <URL where people reported this broken> Fixes: 46dcbc61b739b ("platform/x86: thinkpad-acpi: Add support for automatic mode transitions")
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 1195293b22fd..a95946800ae9 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -10311,9 +10311,11 @@ static DEFINE_MUTEX(dytc_mutex); static int dytc_capabilities; static bool dytc_mmc_get_available; -static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) +static int convert_dytc_to_profile(int funcmode, int dytcmode, + enum platform_profile_option *profile) { - if (dytc_capabilities & BIT(DYTC_FC_MMC)) { + switch (funcmode) { + case DYTC_FUNCTION_MMC: switch (dytcmode) { case DYTC_MODE_MMC_LOWPOWER: *profile = PLATFORM_PROFILE_LOW_POWER; @@ -10329,8 +10331,7 @@ static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *p return -EINVAL; } return 0; - } - if (dytc_capabilities & BIT(DYTC_FC_PSC)) { + case DYTC_FUNCTION_PSC: switch (dytcmode) { case DYTC_MODE_PSC_LOWPOWER: *profile = PLATFORM_PROFILE_LOW_POWER; @@ -10344,6 +10345,14 @@ static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *p default: /* Unknown mode */ return -EINVAL; } + return 0; + case DYTC_FUNCTION_AMT: + /* For now return balanced. It's the closest we have to 'auto' */ + *profile = PLATFORM_PROFILE_BALANCED; + return 0; + default: + /* Unknown function */ + return -EOPNOTSUPP; } return 0; } @@ -10492,6 +10501,7 @@ static int dytc_profile_set(struct platform_profile_handler *pprof, err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_PSC, perfmode, 1), &output); if (err) goto unlock; + /* system supports AMT, activate it when on balanced */ if (dytc_capabilities & BIT(DYTC_FC_AMT)) dytc_control_amt(profile == PLATFORM_PROFILE_BALANCED); @@ -10507,7 +10517,7 @@ static void dytc_profile_refresh(void) { enum platform_profile_option profile; int output, err = 0; - int perfmode; + int perfmode, funcmode; mutex_lock(&dytc_mutex); if (dytc_capabilities & BIT(DYTC_FC_MMC)) { @@ -10522,8 +10532,9 @@ static void dytc_profile_refresh(void) if (err) return; + funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; - convert_dytc_to_profile(perfmode, &profile); + convert_dytc_to_profile(funcmode, perfmode, &profile); if (profile != dytc_current_profile) { dytc_current_profile = profile; platform_profile_notify();
Recently AMT mode was enabled (somewhat unexpectedly) on the Lenovo Z13 platform. The FW is advertising it is available and the driver tries to use it - unfortunately it reports the profile mode incorrectly. Note, there is also some extra work needed to enable the dynamic aspect of AMT support that I will be following up with; but more testing is needed first. This patch just fixes things so the profiles are reported correctly. Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)