Message-ID: <6eeaf114-14bd-4fe2-9359-6b953dcd8bb5@kernel.org>
Date: Mon, 3 Nov 2025 15:33:59 -0600
From: "Mario Limonciello (AMD) (kernel.org)" <superm1@...nel.org>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, Hans de Goede <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Derek John Clark <derekjohn.clark@...il.com>,
 Joaquín Ignacio Aramendía <samsagax@...il.com>,
 Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v3 6/6] platform/x86: ayaneo-ec: Add suspend hook



On 11/3/2025 3:20 PM, Antheas Kapenekakis wrote:
> On Mon, 3 Nov 2025 at 17:51, Mario Limonciello (AMD) (kernel.org)
> <superm1@...nel.org> wrote:
>>
>>
>>
>> On 10/31/2025 11:36 AM, Antheas Kapenekakis wrote:
>>> The Ayaneo EC resets after hibernation, losing the charge control state.
>>> Add a small PM hook to restore this state on hibernation resume.
>>>
>>> The fan speed is also lost during hibernation, but since hibernation
>>> failures are common with this class of devices, setting a low fan speed
>>> when the userspace program controlling the fan will potentially not
>>> take over could cause the device to overheat, so it is not restored.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>> ---
>>>    drivers/platform/x86/ayaneo-ec.c | 73 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 73 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/ayaneo-ec.c b/drivers/platform/x86/ayaneo-ec.c
>>> index 9548e3d22093..e1ad5968d3b4 100644
>>> --- a/drivers/platform/x86/ayaneo-ec.c
>>> +++ b/drivers/platform/x86/ayaneo-ec.c
>>> @@ -41,6 +41,8 @@
>>>    #define AYANEO_MODULE_LEFT  BIT(0)
>>>    #define AYANEO_MODULE_RIGHT BIT(1)
>>>
>>> +#define AYANEO_CACHE_LEN     1
>>> +
>>>    struct ayaneo_ec_quirk {
>>>        bool has_fan_control;
>>>        bool has_charge_control;
>>> @@ -51,6 +53,9 @@ struct ayaneo_ec_platform_data {
>>>        struct platform_device *pdev;
>>>        struct ayaneo_ec_quirk *quirks;
>>>        struct acpi_battery_hook battery_hook;
>>> +
>>> +     bool restore_charge_limit;
>>> +     bool restore_pwm;
>>>    };
>>>
>>>    static const struct ayaneo_ec_quirk quirk_fan = {
>>> @@ -207,10 +212,14 @@ static int ayaneo_ec_read(struct device *dev, enum hwmon_sensor_types type,
>>>    static int ayaneo_ec_write(struct device *dev, enum hwmon_sensor_types type,
>>>                           u32 attr, int channel, long val)
>>>    {
>>> +     struct ayaneo_ec_platform_data *data = platform_get_drvdata(
>>> +             to_platform_device(dev));
>>> +     int ret;
>>>        switch (type) {
>>>        case hwmon_pwm:
>>>                switch (attr) {
>>>                case hwmon_pwm_enable:
>>> +                     data->restore_pwm = false;
>>>                        switch (val) {
>>>                        case 1:
>>>                                return ec_write(AYANEO_PWM_ENABLE_REG,
>>> @@ -224,6 +233,15 @@ static int ayaneo_ec_write(struct device *dev, enum hwmon_sensor_types type,
>>>                case hwmon_pwm_input:
>>>                        if (val < 0 || val > 255)
>>>                                return -EINVAL;
>>> +                     if (data->restore_pwm) {
>>> +                             // Defer restoring PWM control to after
>>> +                             // userspace resumes successfully
>>> +                             ret = ec_write(AYANEO_PWM_ENABLE_REG,
>>> +                                            AYANEO_PWM_MODE_MANUAL);
>>> +                             if (ret)
>>> +                                     return ret;
>>> +                             data->restore_pwm = false;
>>> +                     }
>>>                        return ec_write(AYANEO_PWM_REG, (val * 100) / 255);
>>>                default:
>>>                        break;
>>> @@ -474,10 +492,65 @@ static int ayaneo_ec_probe(struct platform_device *pdev)
>>>        return 0;
>>>    }
>>>
>>> +static int ayaneo_freeze(struct device *dev)
>>> +{
>>> +     struct platform_device *pdev = to_platform_device(dev);
>>> +     struct ayaneo_ec_platform_data *data = platform_get_drvdata(pdev);
>>> +     int ret;
>>> +     u8 tmp;
>>> +
>>> +     if (data->quirks->has_charge_control) {
>>> +             ret = ec_read(AYANEO_CHARGE_REG, &tmp);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             data->restore_charge_limit = tmp == AYANEO_CHARGE_VAL_INHIBIT;
>>> +     }
>>> +
>>> +     if (data->quirks->has_fan_control) {
>>> +             ret = ec_read(AYANEO_PWM_ENABLE_REG, &tmp);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             data->restore_pwm = tmp == AYANEO_PWM_MODE_MANUAL;
>>
>> Why bother with the temp variable in the first place?
>>
>> You could just make the data type of restore_pwm a u8 and then:
>>
>> ec_read(AYANEO_PWM_ENABLE_REG, data->restore_pwm);
> 
> For restore_pwm it needs to be a bool because it is applied lazily on
> resume only if manual. charge limit could be a u8 (it was on the
> previous patch) but I chose to do a bool to match restore_pwm and so
> that I also only apply it selectively.

But you can interpret a u8 as a boolean as well was my point.  If it's 0 
it's false, if it's anything else it's true.

But I'm not gonna die on this hill, just wanted to point it out.

> 
>>
>>> +
>>> +             // Release the fan when entering hibernation to avoid
>>> +             // overheating if hibernation fails and hangs
>>
>> Multi-line comments should be done with /* */
>>
>>> +             if (data->restore_pwm) {
>>> +                     ret = ec_write(AYANEO_PWM_ENABLE_REG, AYANEO_PWM_MODE_AUTO);
>>> +                     if (ret)
>>> +                             return ret;
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int ayaneo_restore(struct device *dev)
>>> +{
>>> +     struct platform_device *pdev = to_platform_device(dev);
>>> +     struct ayaneo_ec_platform_data *data = platform_get_drvdata(pdev);
>>> +     int ret;
>>> +
>>> +     if (data->quirks->has_charge_control && data->restore_charge_limit) {
>>> +             ret = ec_write(AYANEO_CHARGE_REG, AYANEO_CHARGE_VAL_INHIBIT);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops ayaneo_pm_ops = {
>>> +     .freeze = ayaneo_freeze,
>>> +     .restore = ayaneo_restore,
>>> +};
>>
>> Why are freeze and restore special?  Userspace is frozen for the suspend
>> sequence of any flow.  Hangs could happen in suspend just like they can
>> in hibernate.  If you're going to protect users from this I would expect
>> parity for "regular" suspend/resume.
>>
>> Can you just use SIMPLE_DEV_PM_OPS and rename the functions accordingly?
> 
> Well, the ops here do two functions. First, they restore fan and
> charge limiting state, which is only required for hibernation (both
> are maintained during sleep).
> 
> Second, they ensure from entry to exit there is an automatic fan
> curve. For hibernation, the failure rate is 30%-80% depending on
> kernel version and userspace load (incl. which devices such as GPU are
> loaded and how much). Both entry and exit can fail equally. In which
> case the device may be stuck with an inappropriate fan speed for
> minutes. Moreover, even without a failure, hibernation entry and exit
> take around 1-2 minutes to complete so it is a nice touch to release
> the manual speed for entry to maintain a reasonable fan speed.
> 
> For sleep, it is different. It always works, 

Having spent enough time looking at sleep problems I would never make a 
statement like that.  I try really hard to stay on on top of it, but the 
reality is regressions happen all the time.

> so there is no failure
> rate. Then, it requires around 3 seconds for entry and 2 seconds for
> exit, so for successful entry and exit using an automatic fan speed is
> not needed. Introducing restoring auto speed a failsafe risks
> introducing a user-visible flaw where the fan would spike before and
> after sleep. It could potentially introduce other bugs as it does
> unnecessary writes. So this is not a good reason for introducing this.

The other thing to keep in mind is that regressions can happen in 
firmware too, and this is why I generally feel it's best to be 
conservative around sleep states in this area.

I would never tell someone to do it, but technically you can unbind the 
lps0 device.  If this happens what happens to this fan curve stuff? 
Userspace will be frozen and the hardware won't got to a hardware sleep 
state.
> 
> So ops are not required for sleep for either reason they were
> implemented for hibernation
> 
> Ack on the rest
> 
> Antheas
> 
>>> +
>>>    static struct platform_driver ayaneo_platform_driver = {
>>>        .driver = {
>>>                .name = "ayaneo-ec",
>>>                .dev_groups = ayaneo_ec_groups,
>>> +             .pm = &ayaneo_pm_ops,
>>>        },
>>>        .probe = ayaneo_ec_probe,
>>>    };
>>
>>
> 

Powered by blists - more mailing lists