Message-ID: 
 <CAGwozwGAkgXE0zSPLLAPvWmwsRDgRnTLUi5X4zqYCnG3ot0exw@mail.gmail.com>
Date: Tue, 28 Oct 2025 18:49:32 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Armin Wolf <W_Armin@....de>
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

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

> 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