Re: [PATCH 1/6] platform/x86: msi-wmi-platform: Move guard out of switch in platform_write()

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

On 10/24/26 04:24, Antheas Kapenekakis wrote:
> On Wed, 21 Oct 2026 at 05:26, Derek John Clark
> <derekjohn.clark@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2026 at 2:53 PM Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>
>>> On Tue, 18 Aug 2026 at 17:26, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>>
>>>> On Tue, 18 Aug 2026 at 16:42, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>> On 8/18/26 07:12, Ilpo Järvinen wrote:
>>>>>> On Mon, 3 Aug 2026, Derek J. Clark wrote:
>>>>>>
>>>>>>> The ML patch set uses a guard within a switch case that violates cleanup
>>>>>>> rules. Move the guard outside the switch so all return paths are covered.
>>>>>>>
>>>>>>> Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
>>>>>>> ---
>>>>>>>    drivers/platform/x86/msi-wmi-platform.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
>>>>>>> index dfb65ac8fbf6..33acd448f674 100644
>>>>>>> --- a/drivers/platform/x86/msi-wmi-platform.c
>>>>>>> +++ b/drivers/platform/x86/msi-wmi-platform.c
>>>>>>> @@ -675,11 +675,11 @@ static int msi_wmi_platform_write(struct device *dev, enum hwmon_sensor_types ty
>>>>>>>       u8 buffer[32] = { };
>>>>>>>       int ret;
>>>>>>>
>>>>>>> +    guard(mutex)(&data->wmi_lock);
>>>>>>>       switch (type) {
>>>>>>>       case hwmon_pwm:
>>>>>>>               switch (attr) {
>>>>>>>               case hwmon_pwm_enable:
>>>>>>> -                    guard(mutex)(&data->wmi_lock);
>>>>>>
>>>>>> While I cannot see the code easily, one could also add braces to inside
>>>>>> the case to avoid this problem.
>>>>>>
>>>>>
>>>>> I don't see the code either, but wouldn't this be a use case for
>>>>> scoped_guard() ?
>>>>
>>>> Hi, not explicitly stated in the cover letter, but this is essentially
>>>> a series on top of the series I sent last year but I did not clean up
>>>> due to a variety of issues with MSI firmware that would have made the
>>>> driver unsuitable for upstreaming (to my standards at least). These
>>>> issues span Intel driver problems, HID controller instability, and
>>>> varying WMI implementations esp. in regards to PLx setting between AMD
>>>> / Intel variants, shift mode behavior not acting correctly, and fan
>>>> curve application / resetting being weird (the fan curve on off bit
>>>> does not seem to change whether an automatic curve is used but how
>>>> close the device alings to the set curve? requiring us to back up and
>>>> restore the original curve? half of the devices keep the curve during
>>>> sleep and half don't? but if the fan curve is so finicky do we really
>>>> want a resume handler for it? Perhaps we can kick the timing quirks to
>>>> userspace)
>>>>
>>>> For this reason, I focused on other manufacturers such as Ayaneo and
>>>> OneXPlayer. I will revisit this series per request from Derek as he
>>>> seems to want this for steamos and hid-msi is already queued for 7.3?
>>>> Although, to be honest, compared to the controller firmware issues,
>>>> the WMI implementation is pristine.
>>>>
>>>> So I will need to go through these issues 1 by 1 and see which can be
>>>> fixed and either drop certain features in the beginning.
>>>>
>>>> Particularly for the fan curves, if we say that resume handlers are
>>>> required to be in the kernel moving forward, I do not know how
>>>> realistic it is to make those reliable for this WMI device family.
>>>
>>> This driver still needs work. I am getting bug reports about
>>> inconsistent TDP application on the EX. A8 is broken as well. Even
>>> with your patches Derek. I need to grab a new version of center M and
>>> see what they are doing.
>>>
>>> Does this driver work in your experience? What is that SMT toggle you added
>>
>> I've got a few reports of some issues, but nothing with much detail. I
>> have the 8 AI+, 8 EX AI+, and A8 in hand and all three had effective
>> TDP changes in SteamOS with these patches applied and SteamOS Manager.
>> I haven't done extensive testing over multiple suspends or longer
>> sessions myself. I'm also not aware if there have been BIOS updates
>> since my testing.
>>
>> By SMT toggle i think you mean the STAPM toggle for the A8? It was
>> referenced in API_UserScenario.dll (setPowerLimit_AMDSTAMP) which I
>> reviewed using ILSpy. After toggling that TDP started reporting
>> correctly and being settable.
>>
>> Hope this helps. LMK if you need anything tested or additional info.
> 
> Ok, I asked for more feedback. Yes, TDP changes do work, but they are
> not correct. Users AB testing with a Decky plugin called
> SimpleDeckyTDP report major divergence.
> 
> SimpleDeckyTDP uses a combination of RyzenAdj for AMD and RAPL for
> Intel. When set to 7W, users report 25W power consumption when using
> the kernel driver and 15W when using simpledeckytdp on A8. 15W is the
> correct value for a 7W envelope on an X840u APU.
> 
> STAPM should not be required to use or deviate from STT. Both of them
> should have equivalent behavior and match ryzenadj as the underlying
> DPTC hooks are the same, ryzenadj just accesses the mailbox directly.
> Using STAPM is a workaround for buggy vendor firmware that does not
> control boost values correctly (SPPT/FPPT) and is what the first
> generation Lenovo Legion Go did as well before they fixed it.
> 
> STAPM as a workaround works because it uses timers to control boost
> values, so they decay after 2-3 minutes to the steady state TDP set by
> the firmware. STT boosts for as long as the device thermal envelope
> allows. STAPM is deprecated by AMD, only STT should be used going
> forward. +CC Mario

It's up to the OEM to decide whether they have programmed their firmware 
to to use STT or STAPM.  They should not be used at the same time.

If their BIOS/EC is using one and reverse engineered software is using 
the other, you will expect problems.

> 
> This points to a BIOS firmware issue. You should consider getting in
> touch with your MSI contact for feedback and to plan a fix. I do not
> have an MSI contact. And yes, I am not happy my users have to rely on
> third party unvetted plugins for basic power controls.
> 
> But we cannot revisit this driver while basic behavior. does not work.
> If MSI does not fix this or there isn't a bug in this driver we can
> fix, the only viable way forward from our side is to move power
> controls to the DPTC RFC driver/RAPL for affected models and only
> retain fan curves on this driver. I do not want to do this, I have
> userspace integration for this driver already, but it is not
> functional to the standard expected by users.
> 
> I checked the ACPI firmware and unlike other manufacturers, there is
> no EC independent control of the power envelope. We would still need
> to check a low battery hook in firmware but other than that it is a
> straight translation.
> 
> Antheas