Message-ID: <3a3f7625-81ab-4cdf-b406-4b83a4494700@gmx.de>
Date: Wed, 29 Oct 2025 00:14:06 +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 18:49 schrieb Antheas Kapenekakis:

> On Tue, 28 Oct 2025 at 16:26, Armin Wolf <W_Armin@....de> wrote:
>> 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.
> Well, unless you can solve the case of hibernation failing and what
> happens to the fan speed I cannot sign off on it. These devices are
> very expensive.
>
> Controlling the fan is basic functionality, it cannot be marked or be
> made an unsafe feature.
>
> I am aware of the issue post hibernation, but thats easy to solve
> through software, and as a maintainer of fan control software, which
> also happens to be the main consumer of this module, I already have a
> hook there. As it is needed by the other manufacturers as well.
>
> Antheas

Alright, if Guenter is fine with this approach then i wont complain any longer.
However please document this non-standard behavior so people do not accidentally
"fix" it in the future.

Thank,
Armin Wolf

>> 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