Re: [PATCH v5 3/3] hwmon: (applesmc) Convert to hwmon_device_register_with_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The new `pwmX_enable` attribute implementation violates the standard hwmon ABI by mapping automatic mode to `0` and explicitly rejecting `2`.
--

--- Patch [3]: [PATCH v5 3/3] hwmon: (applesmc) Convert to hwmon_device_register_with_info ---
commit 5c6fadffca7be1d46d146ede611dc2b9077f542a
Author: Shih-Yuan Lee <fourdollars@debian.org>

hwmon: (applesmc) Convert to hwmon_device_register_with_info

This commit converts the legacy hwmon_device_register() function in the
applesmc driver to the modern hwmon_device_register_with_info() API. It
dynamically allocates standard hwmon channels and standardizes attribute
naming, such as mapping the legacy fanX_manual attributes to the standard
pwmX_enable ABI.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 317135fc4b73b..bec9e9e988969 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c

[ ... ]

> @@ -1321,8 +1186,238 @@ static const struct dmi_system_id applesmc_whitelist[] __initconst = {
> +static int applesmc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long *val)
> +{
> +	int ret;
> +
> +	switch (type) {

[ ... ]

> +	case hwmon_pwm:
> +		if (attr == hwmon_pwm_enable) {
> +			u8 buffer[2];
> +
> +			ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> +			if (ret)
> +				return ret;
> +			*val = ((buffer[0] << 8 | buffer[1]) >> channel) & 0x01;

[Severity: High]
Does returning 0 when the fan is in automatic control mode conflict with the
standard hwmon ABI?

The standard hwmon ABI defines 2 as automatic control, while 0 typically
means 'no control' or 'full speed'. Returning 0 here might cause standard
userspace tools like fancontrol or pwmconfig from lm-sensors to incorrectly
assume the fan has no speed control.

> +			return 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +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_pwm:
> +		if (attr == hwmon_pwm_enable) {
> +			u8 buffer[2];
> +			u16 manual_val;
> +			const struct applesmc_entry *entry;
> +
> +			if (val != 0 && val != 1)
> +				return -EINVAL;

[Severity: High]
Will rejecting standard hwmon ABI values like 2 break userspace tools?

By explicitly returning -EINVAL for values other than 0 or 1, attempts to
restore automatic mode by writing 2 (the standard hwmon ABI value for
automatic control) will fail. This could leave the fan stuck in manual mode
at an arbitrary speed, risking system overheating.

Since this patch standardizes the attribute name to pwmX_enable, should this
function accept 2 and translate it into the hardware's automatic mode?

> +
> +			entry = applesmc_get_entry_by_key(FANS_MANUAL);
> +			if (IS_ERR(entry))
> +				return PTR_ERR(entry);

[ ... ]

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