Re: [RFC PATCH 1/1] hwmon: Add Minisforum UM780 XTX EC monitoring and fan control

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver implements a custom mutex for sysfs attributes and background work instead of using the required hwmon_lock(), and performs redundant locking inside the core read callback.
--

--- Patch [1]: [RFC PATCH 1/1] hwmon: Add Minisforum UM780 XTX EC monitoring and fan control ---
commit 82feeb1647789c448c800c2c55cd4d4766858bc1
Author: Sebastián Peyrott <[email protected]>

hwmon: Add Minisforum UM780 XTX EC monitoring and fan control

This patch adds a DMI-gated hwmon driver for the embedded controller used by
the Minisforum UM780 XTX. It exposes CPU and system fan control temperatures
and tachometers via sysfs. It also caches coherent fan settings and restores
them after the firmware reloads defaults following resume.

> diff --git a/drivers/hwmon/minisforum-um780xtx.c b/drivers/hwmon/minisforum-um780xtx.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwmon/minisforum-um780xtx.c

[ ... ]

> +struct um780xtx_data {
> +	struct device *dev;
> +	/* Serializes EC transactions and cached state updates. */
> +	struct mutex lock;

[Severity: Medium]
Does this driver need a custom mutex? The hardware monitoring subsystem
guidelines state that drivers should use hwmon_lock() and hwmon_unlock()
to serialize attributes registered by other means and for background
handling. Using a custom lock might fragment synchronization and race with
the hwmon core's serialization.

[ ... ]

> +static ssize_t pwm1_enable_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct um780xtx_data *data = dev_get_drvdata(dev);
> +	unsigned long mode;
> +	u8 expected;
> +	u8 profile;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &mode);
> +	if (ret)
> +		return ret;
> +	if (mode != 2 && mode != 3)
> +		return -EINVAL;
> +	expected = mode == 2 ? UM780XTX_EC_PROFILE_B1 : UM780XTX_EC_PROFILE_B2;
> +
> +	mutex_lock(&data->lock);

[Severity: Medium]
Should this use hwmon_lock() instead of the private data->lock? Since
this attribute is registered outside the core hwmon info array (via
extra_groups), the subsystem guidelines require using hwmon_lock() here to
properly synchronize with core operations.

[ ... ]

> +static int um780xtx_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *value)
> +{
> +	struct um780xtx_data *data = dev_get_drvdata(dev);
> +	u8 raw;
> +	int ret;
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_input && channel < 2) {
> +		mutex_lock(&data->lock);

[Severity: Medium]
Is this lock acquisition redundant? The hardware monitoring core already
serializes operations for attributes registered via the info parameter of
hwmon_device_register_with_info(). Acquiring another lock inside the core
read callback might be unnecessary if hwmon_lock() is used throughout.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1