Re: [PATCH v6 2/3] hwmon: it87: prepare for extended PWM temp maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] it87_check_pwm corrupts the PWM control register on newer controllers when fixing broken BIOS polarity.
- [High] User-configured temperature mappings are silently discarded when switching legacy controllers to automatic PWM mode.
--

commit e4890144d7762af5309a78a9545773b57261f124
Author: benoit.masson <yahoo@perenite.com>
hwmon: it87: prepare for extended PWM temp maps

Introduce helper logic for PWM-to-temperature mappings so newer register
layouts can be supported while retaining the legacy two groups of three
temperature sources.

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 1af815b5d33a9..980e425fcc28b 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
[ ... ]
> @@ -1635,27 +1748,30 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
[ ... ]
>  	} else {
>  		u8 ctrl;
>  
>  		if (has_newer_autopwm(data)) {
> -			ctrl = (data->pwm_ctrl[nr] & 0x7c) |
> -				data->pwm_temp_map[nr];
> +			ctrl = pwm_temp_map_set(data,
> +						data->pwm_ctrl[nr] & ~0x80,
> +						data->pwm_temp_map[nr]);
>  			if (val != 1)
>  				ctrl |= 0x80;
>  		} else {
>  			ctrl = (val == 1 ? data->pwm_duty[nr] : 0x80);
>  		}
>  		data->pwm_ctrl[nr] = ctrl;
> -		it87_write_value(data, IT87_REG_PWM[nr], ctrl);
> +		it87_write_value(data, it87_reg_pwm(data, nr), ctrl);

[Severity: High]
This is a pre-existing issue, but does switching legacy controllers to
automatic pwm mode permanently wipe out the user's deferred temperature
mapping?

When set_pwm_enable() executes the legacy controller branch for automatic
mode (when val is 2), it unconditionally sets ctrl to 0x80. Writing this
value to the pwm control register clears bits 0-1, which discards any
user-configured temperature mappings previously deferred in
data->pwm_temp_map[nr]. Can this logic error lead to fans monitoring the
wrong temperature sensor (defaulting to 0) and potentially causing
hardware overheating?

[ ... ]
> @@ -3493,7 +3596,8 @@ static int it87_check_pwm(struct device *dev)
[ ... ]
>  			if (!((pwm[0] | pwm[1] | pwm[2]) & 0x80)) {
>  				dev_info(dev,
>  					 "Reconfiguring PWM to active high polarity\n");
>  				it87_write_value(data, IT87_REG_FAN_CTL,
>  						 tmp | 0x87);
>  				for (i = 0; i < 3; i++)
>  					it87_write_value(data,
> -							 IT87_REG_PWM[i],
> +							 it87_reg_pwm(data,
> +								      i),
>  							 0x7f & ~pwm[i]);

[Severity: High]
This isn't a bug introduced by this patch, but does this write corrupt
the temperature mapping on newer controllers?

When fixing broken BIOS polarity in it87_check_pwm(), writing 0x7f & ~pwm[i]
inverts the manual duty cycle bits for older controllers. However, on newer
controllers the duty cycle is in a separate register, and the pwm control
register instead holds the temperature mapping. Does inverting these bits
corrupt the temperature mapping and cause incorrect fan behavior?

>  				return 1;
>  			}

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