| 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: <0483bdf2-c1a2-4a8e-b0ee-b00cafdda557@t-8ch.de>
Date: Wed, 30 Apr 2025 16:48:15 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Sung-Chi Li <lschyi@...omium.org>
Cc: Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>, Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>,
chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans
On 2025-04-30 15:00:10+0800, Sung-Chi Li wrote:
> On Tue, Apr 29, 2025 at 11:20:09PM +0200, Thomas Weißschuh wrote:
> > On 2025-04-29 16:14:22+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@...omium.org>
> > >
> > > Newer EC firmware supports controlling fans through host commands, so
> > > adding corresponding implementations for controlling these fans in the
> > > driver for other kernel services and userspace to control them.
> > >
> > > The driver will first probe the supported host command versions (get and
> > > set of fan PWM values, get and set of fan control mode) to see if the
> > > connected EC fulfills the requirements of controlling the fan, then
> > > exposes corresponding sysfs nodes for userspace to control the fan with
> > > corresponding read and write implementations.
> > > As EC will automatically change the fan mode to auto when the device is
> > > suspended, the power management hooks are added as well to keep the fan
> > > control mode and fan PWM value consistent during suspend and resume. As
> > > we need to access the hwmon device in the power management hook, update
> > > the driver by storing the hwmon device in the driver data as well.
> > >
> > > Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
> > > ---
> > > Documentation/hwmon/cros_ec_hwmon.rst | 5 +-
> > > drivers/hwmon/cros_ec_hwmon.c | 237 +++++++++++++++++++++++++++++++++-
> > > 2 files changed, 237 insertions(+), 5 deletions(-)
<snip>
> > > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> > > + u8 index, u8 *pwm_value)
> > > +{
> > > + int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
> >
> > The _raw_ function is unnecessary.
> >
>
> This is to share with the `cros_ec_hwmon_cooling_get_cur_state`, and there is a
> unit conversion needed, so extract the same process into a _raw_ function.
What's the advantage of scaling the value for the cooling device?
The hwmon core thermal zone implementation also uses the hwmon values
directly.
> > > +
> > > + if (ret == 0)
> > > + *pwm_value = *pwm_value * 255 / 100;
> > > + return ret;
> > > +}
> > > +
> > > +static int cros_ec_hwmon_read_pwm_enable(struct cros_ec_device *cros_ec,
> > > + u8 index, u8 *control_method)
> > > +{
> > > + struct ec_params_auto_fan_ctrl_v2 req = {
> > > + .fan_idx = index,
> > > + .cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> > > + };
> > > + struct ec_response_auto_fan_control resp;
> > > + int ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req,
> > > + sizeof(req), &resp, sizeof(resp));
> >
> > Keep &foo and sizeof(foo) together on the same line please.
> >
>
> This is automatically formatted by clang-format. I will keep it like this in the
> v2 patch. If it is important for readablity, please share with me, and I will
> update that in the v2 patch.
It's not that important. But unfortunate that clang-format will make the
formatting worse.
<snip>
> > > +static int cros_ec_hwmon_set_pwm_raw_value(struct cros_ec_hwmon_priv *priv,
> > > + u8 index, u8 val)
> > > +{
> > > + int ret;
> > > +
> > > + if (!(priv->manual_fans & BIT(index)))
> > > + return -ECANCELED;
> >
> > Weird error code.
> >
>
> Hmm, do you have some error code suggestion? I think the idea here is we will
> reject to write the PWM value if fan is not in manual mode, and I am not sure
> what error suits for this (is -EPERM the one to use here?).
EOPNOTSUPP maybe.
>
> > > +
> > > + ret = cros_ec_hwmon_write_pwm_value(priv->cros_ec, index, val);
> > > + if (ret == 0)
> > > + priv->manual_fan_pwm_values[index] = val;
> > > + return ret;
> > > +}
<snip>
> > > +static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec,
> > > + u8 index, u8 val)
> > > +{
> > > + struct ec_params_auto_fan_ctrl_v2 req = {
> > > + .fan_idx = index,
> > > + .cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
> >
> > Swap the two lines above.
> >
>
> Will update in v2 patch.
>
> > > + };
> > > + int ret;
> > > +
> > > + /* No CROS EC supports no fan speed control */
> > > + if (val == 0)
> > > + return -EOPNOTSUPP;
> > > +
> > > + req.set_auto = (val != 1) ? true : false;
> > > + ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req,
> > > + sizeof(req), NULL, 0);
> >
> > Use a full 100 columns.
> >
>
> Hmm, I found the style guide actually strongly prefer 80:
> https://www.kernel.org/doc/html/v6.14/process/coding-style.html#breaking-long-lines-and-strings
I don't think it is a strong recommendation anymore.
The hwmon core also doesn't seem to be religious about it.
> > > + if (ret < 0)
> > > + return ret;
> > > + return 0;
> > > +}
> > > +
<snip>
> > > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > > +{
> > > + const struct cros_ec_hwmon_platform_priv *platform_priv =
> > > + dev_get_drvdata(&pdev->dev);
> > > + const struct cros_ec_hwmon_priv *priv =
> > > + dev_get_drvdata(platform_priv->hwmon_dev);
> > > + size_t i;
> > > + int ret;
> > > +
> > > + if (!priv->fan_control_supported)
> > > + return 0;
> > > +
> > > + /*
> > > + * EC sets fan control to auto after suspended, restore settings to
> > > + * before suspended.
> > > + */
> > > + for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > > + if (!(priv->manual_fans & BIT(i)))
> > > + continue;
> >
> > Given that we can read the actual state from the EC I'd prefer to read
> > it back and store it during suspend() instead of storing it during write().
> >
>
> Do you mean reading fan mode and fan PWM value during suspend, or we will keep
> updating `manual_fans` while write(), and do not cache the PWM value while
> write()? That involves whether we need to send a get fan mode for every write
> PWM value.
This one:
"reading fan mode and fan PWM value during suspend"
> > > +
> > > + /*
> > > + * Setting fan PWM value to EC will change the mode to manual
> > > + * for that fan in EC as well, so we do not need to issue a
> > > + * separate fan mode to manual call.
> > > + */
> > > + ret = cros_ec_hwmon_write_pwm_value(
> > > + priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > }
<snip>
Powered by blists - more mailing lists