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

[sashiko-bot@kernel.org]

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