Re: [PATCH v2 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Writing to `hwmon_pwm_enable` causes an immediate deadlock due to recursive acquisition of `smcreg.mutex`.
- [Low] The fallback string for fan labels is incorrectly truncated when read from sysfs due to an unconditional pointer offset.
--

--- Patch [1]: [PATCH v2 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info ---
commit 319a71a13f69cd2d1c53e88a04e2585bea47d8ab
Author: Shih-Yuan Lee <fourdollars@debian.org>

hwmon: (applesmc) Convert to hwmon_device_register_with_info

This patch converts the deprecated hwmon_device_register() function to the
modern hwmon_device_register_with_info() API. It moves the registration of
standard sensor attributes to the HWMON core and dynamically registers
non-standard ones.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 90a14a7f2c4c0..3470912649c4d 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -597,6 +598,16 @@ static int applesmc_init_smcreg_try(void)
>  	if (s->fan_count > 10)
>  		s->fan_count = 10;
>  
> +	for (i = 0; i < s->fan_count; i++) {
> +		char newkey[5];
> +
> +		scnprintf(newkey, sizeof(newkey), FAN_ID_FMT, i);
> +		ret = applesmc_read_key(newkey, s->fan_positions[i], 16);
> +		s->fan_positions[i][16] = 0;
> +		if (ret)
> +			scnprintf(s->fan_positions[i], 17, "Fan %d", i);
> +	}
> +

[ ... ]

> +static int applesmc_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +				u32 attr, int channel, long val)
> +{
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_fan:

[ ... ]

> +		break;
> +	case hwmon_pwm:
> +		if (attr == hwmon_pwm_enable) {
> +			u8 buffer[2];
> +			u16 manual_val;
> +
> +			if (val != 0 && val != 1)
> +				return -EINVAL;
> +			mutex_lock(&smcreg.mutex);
> +			ret = applesmc_read_key(FANS_MANUAL, buffer, 2);

[Severity: High]
Will this cause a deadlock due to recursive mutex acquisition?

The code explicitly acquires smcreg.mutex here, but applesmc_read_key()
and applesmc_write_key() both unconditionally attempt to acquire the same
mutex again through their underlying applesmc_read_entry() and
applesmc_write_entry() calls.

Since this is a standard non-recursive mutex, will this freeze the
calling task when a user writes to hwmon_pwm_enable?

> +			if (ret)
> +				goto out_unlock;
> +			manual_val = (buffer[0] << 8 | buffer[1]);
> +			if (val)
> +				manual_val |= (0x01 << channel);
> +			else
> +				manual_val &= ~(0x01 << channel);
> +			buffer[0] = (manual_val >> 8) & 0xff;
> +			buffer[1] = manual_val & 0xff;
> +			ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> +out_unlock:
> +			mutex_unlock(&smcreg.mutex);
> +			return ret;
> +		}
> +		break;

[ ... ]

> +static int applesmc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +				      u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		if (attr == hwmon_temp_label) {
> +			*str = smcreg.index[channel];
> +			return 0;
> +		}
> +		break;
> +	case hwmon_fan:
> +		if (attr == hwmon_fan_label) {
> +			*str = smcreg.fan_positions[channel] + 4;

[Severity: Low]
Will this unconditionally truncate the fallback string generated during init?

If applesmc_read_key() fails in applesmc_init_smcreg_try(), the driver
generates a fallback string formatted as "Fan %d".

When this sysfs attribute is read, the + 4 pointer arithmetic will skip
the "Fan " prefix and only expose the digit to userspace (e.g., exposing
"0" instead of "Fan 0").

> +			return 0;
> +		}
> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711033700.3693-2-fourdollars@debian.org?part=1