| 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: <e4da28be-66ca-45d3-9ccf-34819460b463@t-8ch.de>
Date: Sat, 22 Mar 2025 14:55:48 +0100
From: Thomas Weißschuh <thomas@...ssschuh.net>
To: Sung-Chi Li <lschyi@...omium.org>
Cc: Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>,
chrome-platform@...ts.linux.dev, linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] hwmon: (cros_ec) Add set and get target fan RPM
function
On 2025-03-18 15:45:23+0800, Sung-Chi Li wrote:
> The ChromeOS embedded controller (EC) supports closed loop fan speed
> control, so add the fan target attribute under hwmon framework, such
> that kernel can expose reading and specifying the desired fan RPM for
> fans connected to the EC.
>
> When probing the cros_ec hwmon module, we also check the supported
> command version of setting target fan RPM. This commit implements the
> version 0 of getting the target fan RPM, which can only read the target
> RPM of the first fan. This commit also implements the version 1 of
> setting the target fan RPM to each fan respectively.
>
> Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
> ---
> ChromeOS embedded controller (EC) supports closed-loop fan control. We
> anticipate to have the fan related control from the kernel side, so this
> series register the HWMON_F_TARGET attribute, and implement the read and
> write function for setting/reading the target fan RPM from the EC side.
Should it be possible to switch back to automatic control?
I can't find anything in the hwmon ABI about it.
And neither in the CrOS EC source.
Am I missing something?
> ---
> Changes in v3:
> - Drop support of v0 setting target fan RPM, thus also simplify
> implementations.
> - Align coding style to existing code, including using if-else rather
> than switch-case, and ensure little endian conversion from read data.
> - Only log warning for failed probing get fan target command version
> instead of fail the whole driver.
> - Link to v2: https://lore.kernel.org/r/20250317-extend_ec_hwmon_fan-v2-1-13670557afe5@chromium.org
>
> Changes in v2:
> - Squash the read, write, and register of fan target attribute to 1
> commit, as they are the same topic.
> - Probe the supported command version from EC for setting the target fan
> RPM, and perform the set fan target RPM based on the supported
> version.
> - Update the used variable type to kernel types (i.e., u32).
> - Link to v1: https://lore.kernel.org/r/20250313-extend_ec_hwmon_fan-v1-0-5c566776f2c4@chromium.org
> ---
> drivers/hwmon/cros_ec_hwmon.c | 90 ++++++++++++++++++++++++++++++++++++++++---
Also update Documentation/hwmon/cros_ec_hwmon.rst .
> 1 file changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 9991c3fa020ac859cbbff29dfb669e53248df885..d54fd85ccb4350fc297bde62a2d98f386ce1a8de 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -21,6 +21,7 @@ struct cros_ec_hwmon_priv {
> struct cros_ec_device *cros_ec;
> const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> u8 usable_fans;
> + int set_fan_target_rpm_version;
> };
>
> static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> @@ -36,6 +37,21 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index
> return 0;
> }
>
> +static int cros_ec_hwmon_read_fan_target(struct cros_ec_device *cros_ec,
> + u32 *speed)
> +{
> + struct ec_response_pwm_get_fan_rpm r;
> + int ret;
> +
> + ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_TARGET_RPM, NULL, 0,
> + &r, sizeof(r));
> + if (ret < 0)
> + return ret;
> +
> + *speed = le32_to_cpu((__force __le32)r.rpm);
> + return 0;
> +}
> +
> static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> {
> unsigned int offset;
> @@ -52,6 +68,31 @@ static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8
> return 0;
> }
>
> +static int cros_ec_hwmon_set_fan_rpm(struct cros_ec_device *cros_ec, u8 index,
> + u16 val)
> +{
> + struct ec_params_pwm_set_fan_target_rpm_v1 req = {
> + .rpm = val,
> + .fan_idx = index,
> + };
> + int ret;
> +
> + ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_TARGET_RPM, &req,
> + sizeof(req), NULL, 0);
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
> +
> +static int cros_ec_hwmon_write_fan(struct cros_ec_hwmon_priv *priv, u32 attr,
> + int channel, long rpm)
> +{
> + if (attr == hwmon_fan_target)
> + return cros_ec_hwmon_set_fan_rpm(priv->cros_ec, channel, rpm);
> + else
> + return -EOPNOTSUPP;
> +}
> +
> static bool cros_ec_hwmon_is_error_fan(u16 speed)
> {
> return speed == EC_FAN_SPEED_NOT_PRESENT || speed == EC_FAN_SPEED_STALLED;
> @@ -75,6 +116,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> {
> struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> int ret = -EOPNOTSUPP;
> + u32 target_rpm;
> u16 speed;
> u8 temp;
>
> @@ -91,6 +133,11 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> if (ret == 0)
> *val = cros_ec_hwmon_is_error_fan(speed);
> + } else if (attr == hwmon_fan_target) {
> + ret = cros_ec_hwmon_read_fan_target(
> + priv->cros_ec, &target_rpm);
Can be on one line.
> + if (ret == 0)
> + *val = target_rpm;
> }
> } else if (type == hwmon_temp) {
> if (attr == hwmon_temp_input) {
> @@ -130,7 +177,15 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
> const struct cros_ec_hwmon_priv *priv = data;
>
> if (type == hwmon_fan) {
> - if (priv->usable_fans & BIT(channel))
> + if (!(priv->usable_fans & BIT(channel)))
> + return 0;
> +
> + if (attr == hwmon_fan_target) {
> + if (priv->set_fan_target_rpm_version == 1)
> + return (channel == 0) ? 0644 : 0200;
> + else
> + return 0;
> + } else
> return 0444;
> } else if (type == hwmon_temp) {
> if (priv->temp_sensor_names[channel])
> @@ -140,13 +195,24 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
> return 0;
> }
>
> +static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> + if (type == hwmon_fan)
> + return cros_ec_hwmon_write_fan(priv, attr, channel, val);
> + else
> + return -EOPNOTSUPP;
> +}
> +
> static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
> HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT,
> - HWMON_F_INPUT | HWMON_F_FAULT),
> + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET),
> HWMON_CHANNEL_INFO(temp,
> HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
> @@ -178,6 +244,7 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
> static const struct hwmon_ops cros_ec_hwmon_ops = {
> .read = cros_ec_hwmon_read,
> .read_string = cros_ec_hwmon_read_string,
> + .write = cros_ec_hwmon_write,
> .is_visible = cros_ec_hwmon_is_visible,
> };
>
> @@ -233,6 +300,18 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
> }
> }
>
> +static void cros_ec_hwmon_probe_fan_target_cmd_version(struct cros_ec_hwmon_priv *priv)
> +{
> + int ret;
> +
> + ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_GET_CMD_VERSIONS);
Spurious space after '='.
This also tests for the wrong command.
> + if (ret < 0) {
> + dev_warn(priv->cros_ec->dev,
> + "error getting target fan RPM set command version: %d\n", ret);
> + }
This could return the error instead, not requiring a new warning.
(The first one of the driver)
> + priv->set_fan_target_rpm_version = (ret > 0) ? 1 : 0;
Check for the specific protocol version that is supported, to be
forward compatible.
The get_fan_target_rpm command is not checked.
Checking for both commands and storing the results in a bool should be
enough.
> +}
> +
> static int cros_ec_hwmon_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -259,6 +338,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>
> cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version);
> cros_ec_hwmon_probe_fans(priv);
> + cros_ec_hwmon_probe_fan_target_cmd_version(priv);
>
> hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> &cros_ec_hwmon_chip_info, NULL);
>
> ---
> base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> change-id: 20250313-extend_ec_hwmon_fan-a5f59aab2cb3
>
> Best regards,
> --
> Sung-Chi Li <lschyi@...omium.org>
>
Powered by blists - more mailing lists