| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e41bb0b3-9c79-4d01-8510-4a60999e238b@gmx.de>
Date: Tue, 28 Oct 2025 14:50:15 +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 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?
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