| 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: <0595e183-e724-4162-902e-298bf41b09ed@amd.com>
Date: Tue, 7 May 2024 09:30:04 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Thomas Weißschuh <linux@...ssschuh.net>,
Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
Benson Leung <bleung@...omium.org>, Lee Jones <lee@...nel.org>
Cc: Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org,
linux-hwmon@...r.kernel.org, chrome-platform@...ts.linux.dev,
Dustin Howett <dustin@...ett.net>, Moritz Fischer <mdf@...nel.org>
Subject: Re: [PATCH 1/2] hwmon: add ChromeOS EC driver
On 5/7/2024 08:57, Thomas Weißschuh wrote:
> The ChromeOS Embedded Controller exposes fan speed and temperature
> readings.
> Expose this data through the hwmon subsystem.
>
> The driver is designed to be probed via the cros_ec mfd device.
>
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
> Documentation/hwmon/cros_ec_hwmon.rst | 26 ++++
> Documentation/hwmon/index.rst | 1 +
> MAINTAINERS | 8 +
> drivers/hwmon/Kconfig | 11 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/cros_ec_hwmon.c | 279 ++++++++++++++++++++++++++++++++++
> 6 files changed, 326 insertions(+)
>
> diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> new file mode 100644
> index 000000000000..aeb88c79d11b
> --- /dev/null
> +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> @@ -0,0 +1,26 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver cros_ec_hwmon
> +===========================
> +
> +Supported chips:
> +
> + * ChromeOS embedded controllers connected via LPC
> +
> + Prefix: 'cros_ec'
> +
> + Addresses scanned: -
> +
> +Author:
> +
> + - Thomas Weißschuh <linux@...ssschuh.net>
> +
> +Description
> +-----------
> +
> +This driver implements support for hardware monitoring commands exposed by the
> +ChromeOS embedded controller used in Chromebooks and other devices.
> +
> +The channel labels exposed via hwmon are retrieved from the EC itself.
> +
> +Fan and temperature readings are supported.
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1ca7a4fe1f8f..355a83e66928 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers
> coretemp
> corsair-cpro
> corsair-psu
> + cros_ec_hwmon
> da9052
> da9055
> dell-smm-hwmon
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c23fda1aa1f0..aa5689169eca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4988,6 +4988,14 @@ S: Maintained
> F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
> F: sound/soc/codecs/cros_ec_codec.*
>
> +CHROMEOS EC HARDWARE MONITORING
> +M: Thomas Weißschuh <thomas@...ssschuh.net>
> +L: chrome-platform@...ts.linux.dev
> +L: linux-hwmon@...r.kernel.org
> +S: Maintained
> +F: Documentation/hwmon/chros_ec_hwmon.rst
> +F: drivers/hwmon/cros_ec_hwmon.c
> +
> CHROMEOS EC SUBDRIVERS
> M: Benson Leung <bleung@...omium.org>
> R: Guenter Roeck <groeck@...omium.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 83945397b6eb..c1284d42697f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU
> This driver can also be built as a module. If so, the module
> will be called corsair-psu.
>
> +config SENSORS_CROS_EC
> + tristate "ChromeOS Embedded Controller sensors"
> + depends on MFD_CROS_EC_DEV
> + default MFD_CROS_EC_DEV
> + help
> + If you say yes here you get support for ChromeOS Embedded Controller
> + sensors.
> +
> + This driver can also be built as a module. If so, the module
> + will be called cros_ec_hwmon.
> +
> config SENSORS_DRIVETEMP
> tristate "Hard disk drives with temperature sensors"
> depends on SCSI && ATA
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5c31808f6378..8519a6b36c00 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
> obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
> +obj-$(CONFIG_SENSORS_CROS_EC) += cros_ec_hwmon.o
> obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> new file mode 100644
> index 000000000000..9588df202a74
> --- /dev/null
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -0,0 +1,279 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ChromesOS EC driver for hwmon
> + *
> + * Copyright (C) 2024 Thomas Weißschuh <linux@...ssschuh.net>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/units.h>
> +
> +#define DRV_NAME "cros-ec-hwmon"
> +
> +struct cros_ec_hwmon_priv {
> + struct cros_ec_device *cros_ec;
> + u8 thermal_version;
> + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> +};
> +
> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> +{
> + int ret;
> + u16 data;
> +
> + if (index >= EC_FAN_SPEED_ENTRIES)
> + return -ENODEV;
I could see an argument that this should be -EINVAL as
EC_FAN_SPEED_ENTRIES is hardcoded in the driver and "index" was passed
into the function.
> +
> + ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> + if (ret < 0)
> + return ret;
> +
> + data = le16_to_cpu(data);
> +
> + if (data == EC_FAN_SPEED_NOT_PRESENT)
> + return -ENODEV;
> +
> + *speed = data;
For safety purposes I think you should either do
if (speed)
*speed = data;
Or have a check at the start of the function and return -EINVAL and
throw up a warning if speed was NULL.
> + return 0;
> +}
> +
> +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
> + u8 index, u8 *data)
> +{
> + unsigned int offset;
> + int ret;
> +
> + if (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES)
> + return -ENODEV;
> +
> + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES)
> + return -ENODEV;
Like cros_ec_hwmon_read_fan_speed I have an opinion these should be -EINVAL.
> +
> + if (index < EC_TEMP_SENSOR_ENTRIES)
> + offset = EC_MEMMAP_TEMP_SENSOR + index;
> + else
> + offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
> +
> + ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
> + if (ret < 0)
> + return ret;
> +
You should make sure that data isn't NULL before doing these checks:
> + if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
> + *data == EC_TEMP_SENSOR_ERROR ||
> + *data == EC_TEMP_SENSOR_NOT_POWERED ||
> + *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> + u16 speed;
> + u8 temp;
> + int ret = -ENODATA;
I feel it is better practice to add this as a 3rd clause than initialize
the variable at the start of the function.
switch (type) {
case hwmon_fan:
//do stuff
break;
case hwmon_temp:
// dot stuff
break;
default:
ret = -ENODATA;
}
return ret;
> +
> + if (type == hwmon_fan) {
> + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> + if (ret == 0)
> + *val = speed;
> + } else if (type == hwmon_temp) {
> + ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp);
> + if (ret == 0)
> + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
> + }
> +
> + return ret;
> +}
> +
> +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id,
> + struct ec_response_temp_sensor_get_info *resp)
> +{
> + int ret;
> + struct {
> + struct cros_ec_command msg;
> + union {
> + struct ec_params_temp_sensor_get_info req;
> + struct ec_response_temp_sensor_get_info resp;
> + } __packed data;
> + } __packed buf = {
> + .msg = {
> + .version = 0,
> + .command = EC_CMD_TEMP_SENSOR_GET_INFO,
> + .insize = sizeof(buf.data.resp),
> + .outsize = sizeof(buf.data.req),
> + },
> + .data.req.id = id,
> + };
> +
> + ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
> + if (ret < 0)
> + return ret;
> +
> + *resp = buf.data.resp;
Make sure that resp isn't NULL.
> + return 0;
> +}
> +
> +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> + int ret = -ENODATA;
How about just drop the variable and then do below two comments:
> +
> + if (type == hwmon_temp && attr == hwmon_temp_label) {
> + *str = priv->temp_sensor_names[channel];
> + ret = 0;
return 0;
> + }
> +
> + return ret;
return -ENODATA;
> +}
> +
> +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + const struct cros_ec_hwmon_priv *priv = data;
> + u16 speed;
> +
> + if (type == hwmon_fan) {
> + if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0)
> + return 0444;
> + } else if (type == hwmon_temp) {
> + if (priv->temp_sensor_names[channel])
> + return 0444;
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_INPUT,
> + HWMON_F_INPUT,
> + HWMON_F_INPUT,
> + HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL),
> + NULL
> +};
> +
> +static const struct hwmon_ops cros_ec_hwmon_ops = {
> + .read = cros_ec_hwmon_read,
> + .read_string = cros_ec_hwmon_read_string,
> + .is_visible = cros_ec_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
> + .ops = &cros_ec_hwmon_ops,
> + .info = cros_ec_hwmon_info,
> +};
> +
> +static int cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv)
> +{
> + struct ec_response_temp_sensor_get_info info;
> + size_t i;
> + u8 temp;
> + int ret;
> +
> + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION,
> + 1, &priv->thermal_version);
> + if (ret < 0)
> + return ret;
> +
> + if (!priv->thermal_version)
> + return 0;
I'm a bit confused; why isn't this -ENODEV? I would think that you
don't want to have probe succeed and make devices if there isn't thermal
support in the EC.
> +
> + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); i++) {
> + if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0)
> + continue;
> +
> + ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info);
> + if (ret < 0)
> + continue;
> +
> + priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s",
> + (int)sizeof(info.sensor_name),
> + info.sensor_name);
> + }
> +
> + return 0;
> +}
> +
> +static int cros_ec_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> + struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> + struct cros_ec_hwmon_priv *priv;
> + struct device *hwmon_dev;
> + int ret;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24);
> +
> + /* Not every platform supports direct reads */
> + if (!cros_ec->cmd_readmem)
> + return -ENOTTY;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->cros_ec = cros_ec;
> +
> + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv);
> + if (ret < 0)
> + return ret;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> + &cros_ec_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct platform_device_id cros_ec_hwmon_id[] = {
> + { DRV_NAME, 0 },
> + { }
> +};
> +
> +static struct platform_driver cros_ec_hwmon_driver = {
> + .driver.name = DRV_NAME,
> + .probe = cros_ec_hwmon_probe,
> + .id_table = cros_ec_hwmon_id,
> +};
> +module_platform_driver(cros_ec_hwmon_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id);
> +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@...ssschuh.net");
> +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists