Message-ID: <b8f94bcd-fec3-4755-9179-044fd0aef36d@gmx.de>
Date: Tue, 28 Oct 2025 16:25:35 +0100
From: Armin Wolf <W_Armin@....de>
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 v2 6/6] platform/x86: ayaneo-ec: Add suspend hook

Am 28.10.25 um 16:20 schrieb Antheas Kapenekakis:

> On Tue, 28 Oct 2025 at 14:50, Armin Wolf <W_Armin@....de> wrote:
>> Am 27.10.25 um 00:17 schrieb Antheas Kapenekakis:
>>
>>> On Sun, 26 Oct 2025 at 23:50, Armin Wolf <W_Armin@....de> wrote:
>>>> Am 15.10.25 um 10:44 schrieb Antheas Kapenekakis:
>>>>
>>>>> 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.
>>>> I am still not happy with potentially breaking fancontrol on this device.
>>>> Most users expect fancontrol to continue working after hibernation, so not
>>>> restoring the fan speed configuration seems risky to me. Would it be enough
>>>> to warn users about his inside the documentation?
>>> This device features two modes of operation: a factory fan curve
>>> managed by the EC and a fixed speed set via override of the EC.
>>>
>>> The factory curve is tuned by the manufacturer to result in safe
>>> operation in all conditions by monitoring the CPU temperature and is
>>> not adjustable.
>>>
>>> The fixed speed, on its own when set manually, is not use-able,
>>> because this device has a fluctuating temperature based on workload.
>>> So to meet the varying conditions, its speed would either have to be
>>> set too high, leading to excess noise, or too low, potentially
>>> overheating. Therefore, users of this interface control it via a
>>> userspace program, e.g., hhd, coolercontrol, which allows creating a
>>> custom fan curve based on measurements of temperature sensors.
>>>
>>> When entering hibernation, the userspace program that controls the fan
>>> speed is frozen, so the fan remains at its previous speed regardless
>>> of temperature readings and there are no safety checks.
>>>
>>> When resuming from hibernation, the EC takes over and monitors the
>>> temperature, so it is safe until the userspace program is thawed. If
>>> we introduce a resume hook, we take over from the EC before the
>>> program is ready, introducing a gap where the device can potentially
>>> overheat. If anything, the freeze hook should remove the fan speed
>>> override instead, because suspend-then-hibernate is more of a
>>> liability for overheating if hibernation hangs.
>> Understandable, how about introducing a module_param_unsafe() for enabling
>> write access to the fan settings? The fan settings would be read-only by default,
>> so no suspend handling would be necessary. Said suspend handling would only be
>> necessary when the user _explicitly_ requests write access to the fan settings.
>>
>> What i am trying to say is that we should either expose a fully working feature
>> (fan control with suspend support) or none at all (fan speed monitoring only).
>>
>> What do you thing about that?
> It is a safe parameter and it works during suspend. It has parity with
> current hwmon drivers for other manufacturers.
>
> Hibernation hooks for hwmon are unprecedented, in addition to
> compromising the safety of the device. They _could_ be justified for
> EC managed curves, since a minority of users might opt to set them via
> systemd udev rules and the EC manages the temperature. But this is not
> the case here.
>
> Where does the need for these hooks stem from?
>
> Antheas

I agree that most hwmon drivers sadly do not restore the fan control settings during
resume from hibernation. This however is an error inside the drivers themself, as device
drivers are normally expected to restore such settings during resume. Without this the
fancontrol software will suddenly stop working after hibernation, something users do no
expect.

Copying the faulty behavior of existing drivers is not a good idea here.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
>>> Other devices feature adjustable EC fan curves (e.g., Lenovo, Asus,
>>> AYN, MSI). Since the EC monitors the temperature there, it is fine to
>>> restore the fan curve. Speaking of, I am having quite a few issues
>>> with MSI Claws, so that series is a bit on the back burner, so I plan
>>> to push these series first.
>>>
>>> I will try to tend to this series in the next days. I wanted to push
>>> the Asus stuff first though.
>>>
>>>
>>> Antheas
>>>
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>>>> ---
>>>>>     drivers/platform/x86/ayaneo-ec.c | 42 ++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/ayaneo-ec.c b/drivers/platform/x86/ayaneo-ec.c
>>>>> index 73e9dd39c703..8529f6f8dc69 100644
>>>>> --- a/drivers/platform/x86/ayaneo-ec.c
>>>>> +++ b/drivers/platform/x86/ayaneo-ec.c
>>>>> @@ -37,6 +37,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;
>>>>> @@ -47,6 +49,8 @@ struct ayaneo_ec_platform_data {
>>>>>         struct platform_device *pdev;
>>>>>         struct ayaneo_ec_quirk *quirks;
>>>>>         struct acpi_battery_hook battery_hook;
>>>>> +
>>>>> +     u8 cache[AYANEO_CACHE_LEN];
>>>>>     };
>>>>>
>>>>>     static const struct ayaneo_ec_quirk quirk_fan = {
>>>>> @@ -464,10 +468,48 @@ 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, i = 0;
>>>>> +
>>>>> +     if (data->quirks->has_charge_control) {
>>>>> +             ret = ec_read(AYANEO_CHARGE_REG, &data->cache[i]);
>>>>> +             if (ret)
>>>>> +                     return ret;
>>>>> +             i++;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int ayaneo_thaw(struct device *dev)
>>>>> +{
>>>>> +     struct platform_device *pdev = to_platform_device(dev);
>>>>> +     struct ayaneo_ec_platform_data *data = platform_get_drvdata(pdev);
>>>>> +     int ret, i = 0;
>>>>> +
>>>>> +     if (data->quirks->has_charge_control) {
>>>>> +             ret = ec_write(AYANEO_CHARGE_REG, data->cache[i]);
>>>>> +             if (ret)
>>>>> +                     return ret;
>>>>> +             i++;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops ayaneo_pm_ops = {
>>>>> +     .freeze = ayaneo_freeze,
>>>>> +     .thaw = ayaneo_thaw,
>>>>> +};
>>>>> +
>>>>>     static struct platform_driver ayaneo_platform_driver = {
>>>>>         .driver = {
>>>>>                 .name = "ayaneo-ec",
>>>>>                 .dev_groups = ayaneo_ec_groups,
>>>>> +             .pm = &ayaneo_pm_ops,
>>>> Please use pm_sleep_ptr() here.
>>>>
>>>> Thanks,
>>>> Armin Wolf
>>>>
>>>>>         },
>>>>>         .probe = ayaneo_ec_probe,
>>>>>     };

Powered by blists - more mailing lists