Re: [PATCH v5 2/4] hwmon: it87: prepare for extended PWM temp maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A missing pair of braces in `it87_update_pwm_ctrl` causes a dangling `else` that unconditionally corrupts the manual PWM duty cycle for legacy chips.
- [High] Adding `it87_update_pwm_ctrl` to `set_pwm_enable` clobbers deferred `pwm_temp_map` settings for newer chips.
- [Medium] Replacing `data->num_temp_map` with hardcoded `IT87_PWM_OLD_NUM_TEMP` breaks PWM-to-temperature mapping on chips with larger map groupings (e.g., IT8603E and IT8622E).
--

commit df4c2373d867162752a6dc57c16ff16987dbedff
Author: benoit.masson <yahoo@perenite.com>

hwmon: it87: prepare for extended PWM temp maps

This commit introduces helper logic for PWM-to-temperature mappings,
preparing the driver for newer register layouts with extended maps
while keeping legacy chips unaffected. It replaces the old hardcoded
3-sensor mappings with dynamic lookup helpers based on chip features.

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 1c77f6c97fc36..e077adbb30911 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
[ ... ]
> @@ -786,6 +816,71 @@ static int pwm_from_reg(const struct it87_data *data, u8 reg)
[ ... ]
> +static unsigned int pwm_temp_channel(const struct it87_data *data,
> +				     int nr, u8 map)
> +{
> +	if (has_new_tempmap(data)) {
> +		u8 num = pwm_num_temp_map(data);
> +
> +		if (map >= num)
> +			map = 0;
> +		return map;
> +	}
> +
> +	if (map >= IT87_PWM_OLD_NUM_TEMP)
> +		map = 0;
> +
> +	if (nr >= IT87_PWM_OLD_NUM_TEMP)
> +		map += IT87_PWM_OLD_NUM_TEMP;

[Severity: Medium]
Does replacing data->num_temp_map with the hardcoded IT87_PWM_OLD_NUM_TEMP
here break chips with 4 temperature maps?

Chips like IT8603E and IT8622E have num_temp_map == 4, but lack the
FEAT_NEW_TEMPMAP flag. This logic unconditionally forces an offset of 3 for
pwm4 on legacy chips, ignoring the fact that they might have 4 maps.

> +
> +	return map;
> +}
> +
> +static int pwm_temp_map_from_channel(const struct it87_data *data, int nr,
> +				     unsigned int channel, u8 *map)
> +{
> +	if (has_new_tempmap(data)) {
> +		u8 num = pwm_num_temp_map(data);
> +
> +		if (channel >= num)
> +			return -EINVAL;
> +		*map = channel;
> +		return 0;
> +	}
> +
> +	if (nr >= IT87_PWM_OLD_NUM_TEMP) {
> +		if (channel < IT87_PWM_OLD_NUM_TEMP ||
> +		    channel >= 2 * IT87_PWM_OLD_NUM_TEMP)
> +			return -EINVAL;
> +		channel -= IT87_PWM_OLD_NUM_TEMP;
> +	} else {

[Severity: Medium]
Will this erroneously reject valid sysfs inputs for pwm4 on chips with
a 4-sensor mapping?

Because IT87_PWM_OLD_NUM_TEMP is hardcoded to 3, this restricts valid
sysfs input for pwm4 to channels 3-5. This rejects channels 0-3, which are
completely valid for a chip with a 4-sensor mapping.

> +		if (channel >= IT87_PWM_OLD_NUM_TEMP)
> +			return -EINVAL;
> +	}
> +
> +	*map = channel;
> +	return 0;
> +}
[ ... ]
> @@ -877,14 +977,22 @@ static void it87_write_value(struct it87_data *data, u8 reg, u8 value)
>  
>  static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
>  {
> -	data->pwm_ctrl[nr] = it87_read_value(data, IT87_REG_PWM[nr]);
> +	data->pwm_ctrl[nr] = it87_read_value(data, it87_reg_pwm(data, nr));
>  	if (has_newer_autopwm(data)) {
> -		data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> +		data->pwm_temp_map[nr] =
> +			pwm_temp_map_get(data, data->pwm_ctrl[nr]);
> +		if (has_new_tempmap(data) &&
> +		    data->pwm_temp_map[nr] >= pwm_num_temp_map(data))
> +			data->pwm_temp_map[nr] = 0;
>  		data->pwm_duty[nr] = it87_read_value(data,
>  						     IT87_REG_PWM_DUTY[nr]);
>  	} else {
>  		if (data->pwm_ctrl[nr] & 0x80)	/* Automatic mode */
> -			data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> +			data->pwm_temp_map[nr] =
> +				pwm_temp_map_get(data, data->pwm_ctrl[nr]);
> +		if (has_new_tempmap(data) &&
> +		    data->pwm_temp_map[nr] >= pwm_num_temp_map(data))
> +			data->pwm_temp_map[nr] = 0;
>  		else				/* Manual mode */
>  			data->pwm_duty[nr] = data->pwm_ctrl[nr] & 0x7f;
>  	}

[Severity: High]
Does this dangling else clause unconditionally corrupt the manual PWM duty
cycle for legacy chips?

Because there are no braces around the first "if (data->pwm_ctrl[nr] & 0x80)",
the "else /* Manual mode */" binds to the second
"if (has_new_tempmap(data)...)".

For legacy chips, has_new_tempmap(data) is always false, causing the else
block to execute unconditionally and corrupt the pwm_duty cache with the
lowest 7 bits of the temperature mapping register when in automatic mode.

When a user subsequently switches back to manual mode, this corrupted near-zero
value is written to the hardware, potentially spinning down the fan and
posing a critical overheating risk.

[ ... ]
> @@ -1617,6 +1725,8 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
>  	if (err)
>  		return err;
>  
> +	it87_update_pwm_ctrl(data, nr);
> +
>  	if (val == 0) {
>  		if (nr < 3 && has_fanctl_onoff(data)) {
>  			int tmp;

[Severity: High]
Can this added call to it87_update_pwm_ctrl() clobber deferred user
settings stored in the software cache?

The driver explicitly allows users to configure pwm_temp_map while in manual
mode, deferring the actual hardware write until the fan is switched to
automatic mode. The added unconditional call to it87_update_pwm_ctrl()
refreshes the software cache from hardware registers right before a mode
transition, permanently overwriting and destroying the user's deferred
configuration.

When automatic mode is enabled, the fan will operate on the old temperature
mapping, potentially reacting to the wrong sensor and causing the hardware
to overheat.

[ ... ]
> @@ -3422,7 +3518,10 @@ static void it87_init_device(struct platform_device *pdev)
>  	 * manual duty cycle.
>  	 */
>  	for (i = 0; i < NUM_AUTO_PWM; i++) {
> -		data->pwm_temp_map[i] = i;
> +		if (has_new_tempmap(data))
> +			data->pwm_temp_map[i] = 0;
> +		else
> +			data->pwm_temp_map[i] = i % IT87_PWM_OLD_NUM_TEMP;
>  		data->pwm_duty[i] = 0x7f;	/* Full speed */
>  		data->auto_pwm[i][3] = 0x7f;	/* Full speed, hard-coded */
>  	}

[Severity: Medium]
Is it intentional that this redefines the default hardware mapping on
boot for chips with 4 temperature maps?

By relying on IT87_PWM_OLD_NUM_TEMP, this breaks the default mapping for
chips like IT8603E and IT8622E which have num_temp_map == 4 but lack the
FEAT_NEW_TEMPMAP flag.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829170545.33401-1-jerome.tollet@gmail.com?part=2