Re: [PATCH 2/3] platform/x86: dasharo-acpi: Add fan mode and PWM write support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] platform/x86: dasharo-acpi: broken per-channel fan mode
  mapping and visibility
--

commit 1345c6559b6f48fc9853d519fa4b3c71e20766e1
Author: Filip Gołaś <filip.golas@3mdeb.com>

platform/x86: dasharo-acpi: Add fan mode and PWM write support

This commit adds hwmon support for switching fans between automatic and
manual control via pwmN_enable and setting PWM duty cycles via pwmN.

> diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
> index 13d97eb43511f..29afcf78564c5 100644
> --- a/drivers/platform/x86/dasharo-acpi.c
> +++ b/drivers/platform/x86/dasharo-acpi.c
[ ... ]
> @@ -162,6 +189,14 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			*val = value;
>  		break;
>  	case hwmon_pwm:
> +		if (attr == hwmon_pwm_enable) {
> +			/* hwmon: 1 = manual, 2 = automatic */
> +			ret = dasharo_read_channel(data, "GFMD", DASHARO_FEATURE_FAN_MODE,
> +						   channel, &value);
> +			if (!ret)
> +				*val = value ? 1 : 2;
> +			break;
> +		}
>  		ret = dasharo_read_channel(data, "GFDC", DASHARO_FEATURE_FAN_PWM, channel, &value);
>  		if (!ret)
>  			*val = value;
> @@ -174,6 +209,31 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  	return ret;
>  }
>  
> +static int dasharo_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long val)
> +{
> +	struct dasharo_data *data = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		if (val != 1 && val != 2)
> +			return -EINVAL;
> +		return dasharo_write_channel(data, "SFMD", DASHARO_FEATURE_FAN_MODE,
> +					     channel, val == 1);
[ ... ]
> @@ -210,9 +270,11 @@ static umode_t dasharo_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_t
>  			return 0444;
>  		break;
>  	case hwmon_pwm:
> -		if (channel < data->caps_found[DASHARO_FEATURE_FAN_PWM])
> -			return 0444;
> -		break;
> +		if (channel >= data->caps_found[DASHARO_FEATURE_FAN_PWM])
> +			break;
> +		if (attr == hwmon_pwm_enable)
> +			return channel < data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0;
> +		return data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0444;

[Severity: Medium]
Can indexing DASHARO_FEATURE_FAN_MODE directly with the hwmon_pwm channel
number operate on the wrong fan when only a subset of PWM fans supports
mode control?

dasharo_fill_feature_caps() packs capabilities for each feature densely
from index 0 across groups, skipping groups with no capabilities.  If an
earlier fan supports DASHARO_FEATURE_FAN_PWM without
DASHARO_FEATURE_FAN_MODE while a later fan supports both,
data->capabilities[DASHARO_FEATURE_FAN_MODE][0] refers to the later fan
while channel 0 of DASHARO_FEATURE_FAN_PWM refers to the earlier one.
dasharo_hwmon_is_visible() will then expose pwm1_enable on the earlier
fan instead of the later one, and GFMD/SFMD will read and switch the mode
of a different fan than GFDC/SFDC.

Also, for hwmon_pwm_input, dasharo_hwmon_is_visible() checks
data->caps_found[DASHARO_FEATURE_FAN_MODE] globally rather than checking
whether the given channel supports DASHARO_FEATURE_FAN_MODE, exposing
pwmN as 0644 instead of 0444 on fans without mode control.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113074347.2301465-1-filip.golas@3mdeb.com?part=2