[PATCH 0/3] hwmon: (cros_ec) Synchronize EC access from the thermal device callbacks

[PATCH 0/3] hwmon: (cros_ec) Synchronize EC access from the thermal device callbacks

[Thomas Weißschuh]

To guarantee consistency for the read-modify-write access in
cros_ec_hwmon_cooling_set_cur_state(), locking is necessary.

Use the existing hwmon device lock to do so.

Sung-Chi: Could you test this? I do not have access to hardware which
triggers this code path.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (3):
      hwmon: (cros_ec) Register the thermal devices after the hwmon ones
      hwmon: (cros_ec) Store the hwmon device in cros_ec_hwmon_priv
      hwmon: (cros_ec) Synchronize EC access from the thermal device callbacks

 drivers/hwmon/cros_ec_hwmon.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
---
base-commit: ca078d004cf58137bcf8cb24a8b271397431ba58
change-id: 20260630-cros_ec-hwmon-locking-d32263db1c94

Best regards,
--  
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/3] hwmon: (cros_ec) Register the thermal devices after the hwmon ones

[Thomas Weißschuh]

To guarantee consistency for the read-modify-write access in
cros_ec_hwmon_cooling_set_cur_state(), locking is necessary.
The locking will use standard hwmon device locks, which requires
the hwmon device to be ready before the fan devices are set up.

Reorder the initialization so this works.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 44291799dd8e..f9b19da50472 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -579,13 +579,17 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	priv->fan_control_supported = cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
 	priv->temp_threshold_supported = is_cros_ec_cmd_available(priv->cros_ec,
 								  EC_CMD_THERMAL_GET_THRESHOLD, 1);
-	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
 							 &cros_ec_hwmon_chip_info, NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
+
 	platform_set_drvdata(pdev, priv);
 
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	return 0;
 }
 
 static int cros_ec_hwmon_suspend(struct platform_device *pdev, pm_message_t state)

-- 
2.55.0

Re: [PATCH 1/3] hwmon: (cros_ec) Register the thermal devices after the hwmon ones

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch claims to prepare for hwmon device locking in thermal callbacks but fails to save the hwmon device pointer and omits the locks.
--

--- Patch [1]: [PATCH 1/3] hwmon: (cros_ec) Register the thermal devices after the hwmon ones ---
commit 99d5e8ee386ab9de9bbd8a3d4c1b2a9515fec3a7
Author: Thomas Weißschuh <linux@weissschuh.net>

hwmon: (cros_ec) Register the thermal devices after the hwmon ones

This patch reorders initialization to set up hwmon devices before fan cooling
devices. It intends to prepare the driver for using standard hwmon device
locks to protect read-modify-write accesses.

> To guarantee consistency for the read-modify-write access in
> cros_ec_hwmon_cooling_set_cur_state(), locking is necessary.
> The locking will use standard hwmon device locks, which requires
> the hwmon device to be ready before the fan devices are set up.

[Severity: Medium]
Are the actual hwmon_lock() and hwmon_unlock() calls missing from this patch?
The commit message indicates locking is being prepared, but
cros_ec_hwmon_cooling_set_cur_state() still seems to perform read-modify-write
operations without acquiring the locks.

> Reorder the initialization so this works.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 44291799dd8e4..f9b19da50472d 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -579,13 +579,17 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  	priv->fan_control_supported = cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
>  	priv->temp_threshold_supported = is_cros_ec_cmd_available(priv->cros_ec,
>  								  EC_CMD_THERMAL_GET_THRESHOLD, 1);
> -	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
>  
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
>  							 &cros_ec_hwmon_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
> +

[Severity: Medium]
How will the thermal callbacks access the hwmon device to perform the locking?

It appears the hwmon_dev pointer is only stored in a local variable here,
rather than being saved to the driver's private data. Without saving it to
something like priv->hwmon_dev, cros_ec_hwmon_cooling_set_cur_state() won't
be able to retrieve the device to call hwmon_lock().

>  	platform_set_drvdata(pdev, priv);
>  
> -	return PTR_ERR_OR_ZERO(hwmon_dev);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-cros_ec-hwmon-locking-v1-0-cb6d0fdbb2d3@weissschuh.net?part=1

[PATCH 2/3] hwmon: (cros_ec) Store the hwmon device in cros_ec_hwmon_priv

[Thomas Weißschuh]

The upcoming locking scheme for thermal devices will require access
to the hwmon device.

Add it do the priv struct.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index f9b19da50472..fb5daa9f80d1 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -25,6 +25,7 @@
 
 struct cros_ec_hwmon_priv {
 	struct cros_ec_device *cros_ec;
+	struct device *hwmon_dev;
 	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
 	u8 usable_fans;
 	bool fan_control_supported;
@@ -556,7 +557,6 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	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;
 	u8 thermal_version;
 	int ret;
 
@@ -580,10 +580,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	priv->temp_threshold_supported = is_cros_ec_cmd_available(priv->cros_ec,
 								  EC_CMD_THERMAL_GET_THRESHOLD, 1);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
-							 &cros_ec_hwmon_chip_info, NULL);
-	if (IS_ERR(hwmon_dev))
-		return PTR_ERR(hwmon_dev);
+	priv->hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
+							       &cros_ec_hwmon_chip_info, NULL);
+	if (IS_ERR(priv->hwmon_dev))
+		return PTR_ERR(priv->hwmon_dev);
 
 	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
 

-- 
2.55.0

Re: [PATCH 2/3] hwmon: (cros_ec) Store the hwmon device in cros_ec_hwmon_priv

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A TOCTOU race condition exists between thermal core cooling state updates and hwmon sysfs attribute writes, leading to the EC fan control mode being unintentionally overridden from auto back to manual.
--

--- Patch [2]: [PATCH 2/3] hwmon: (cros_ec) Store the hwmon device in cros_ec_hwmon_priv ---
commit 7ca9614b2a60a2ddf82816080be1dcfa8ab66da6
Author: Thomas Weißschuh <linux@weissschuh.net>

hwmon: (cros_ec) Store the hwmon device in cros_ec_hwmon_priv

This patch adds a pointer to the hwmon device within the cros_ec_hwmon_priv
structure. The change prepares the driver for an upcoming locking scheme
for thermal devices that requires access to the hwmon device.

> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index f9b19da50472d..fb5daa9f80d1c 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -25,6 +25,7 @@
>  
>  struct cros_ec_hwmon_priv {
>  	struct cros_ec_device *cros_ec;
> +	struct device *hwmon_dev;

[Severity: High]
This is a pre-existing issue, but since this patch prepares the driver for
an upcoming thermal locking scheme, is there a TOCTOU race condition during
PWM updates without hwmon_lock() serialization?

Looking at cros_ec_hwmon_write_pwm_input(), which can be called by the thermal
core to update the cooling state:

static int cros_ec_hwmon_write_pwm_input(struct cros_ec_device *cros_ec, u8 index, u8 val)
{
	u8 control_method;
	int ret;

	ret = cros_ec_hwmon_read_pwm_enable(cros_ec, index, &control_method);
	if (ret)
		return ret;
	if (control_method != 1)
		return -EOPNOTSUPP;

	return cros_ec_hwmon_set_fan_pwm_val(cros_ec, index, val);
}

If a concurrent user-space thread writes to the hwmon_pwm_enable sysfs
attribute to set the mode to auto after the control_method check, the EC
successfully transitions to auto.

When the thermal core thread resumes and writes the PWM value via
cros_ec_hwmon_set_fan_pwm_val(), the EC implicitly reverts the fan control
mode back to manual.

If the mode is overridden back to manual after user-space successfully set it
to auto, the fan could remain stuck at a low or zero PWM speed. Does this
path need to be protected by hwmon_lock() to ensure the fan control mode
is not unintentionally altered?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-cros_ec-hwmon-locking-v1-0-cb6d0fdbb2d3@weissschuh.net?part=2

[PATCH 3/3] hwmon: (cros_ec) Synchronize EC access from the thermal device callbacks

[Thomas Weißschuh]

To guarantee consistency for the read-modify-write access in
cros_ec_hwmon_cooling_set_cur_state(), locking is necessary.

Use the existing hwmon device lock to do so.

Fixes: 5798b62867b4 ("hwmon: (cros_ec) register fans into thermal framework cooling devices")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index fb5daa9f80d1..76d83e1c68cf 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -5,6 +5,7 @@
  *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/hwmon.h>
 #include <linux/math.h>
@@ -411,6 +412,8 @@ static int cros_ec_hwmon_cooling_get_cur_state(struct thermal_cooling_device *cd
 	u8 read_val;
 	int ret;
 
+	guard(hwmon_lock)(priv->hwmon_priv->hwmon_dev);
+
 	ret = cros_ec_hwmon_read_pwm_value(priv->hwmon_priv->cros_ec, priv->index, &read_val);
 	if (ret)
 		return ret;
@@ -424,6 +427,8 @@ static int cros_ec_hwmon_cooling_set_cur_state(struct thermal_cooling_device *cd
 {
 	const struct cros_ec_hwmon_cooling_priv *priv = cdev->devdata;
 
+	guard(hwmon_lock)(priv->hwmon_priv->hwmon_dev);
+
 	return cros_ec_hwmon_write_pwm_input(priv->hwmon_priv->cros_ec, priv->index, val);
 }
 

-- 
2.55.0

Re: [PATCH 3/3] hwmon: (cros_ec) Synchronize EC access from the thermal device callbacks

[sashiko-bot]

> To guarantee consistency for the read-modify-write access in
> cros_ec_hwmon_cooling_set_cur_state(), locking is necessary.
> 
> Use the existing hwmon device lock to do so.
> 
> Fixes: 5798b62867b4 ("hwmon: (cros_ec) register fans into thermal framework cooling devices")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-cros_ec-hwmon-locking-v1-0-cb6d0fdbb2d3@weissschuh.net?part=3