Re: [PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver fails to check if `i2c_smbus_read_byte_data()` returns a negative error code before assigning the result to an unsigned 8-bit variable, leading to hardware register corruption on I2C failure.
--

commit 280d2e57aec56ef87aeac8e747586012f93f520e
Author: Jan-Henrik Bruhn <kernel@jhbruhn.de>

hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

This commit exposes the PWM frequency and LUT hysteresis as writable attributes
for the LM63 hardware monitoring driver. It adds pwm1_freq and a single
chip-wide pwm1_auto_point_temp_hyst attribute to configure the fan control.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..3c9e65e3d81db 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
[ ... ]
> @@ -455,6 +458,75 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
[ ... ]
> +static ssize_t pwm1_freq_store(struct device *dev,
> +			       struct device_attribute *dummy,
> +			       const char *buf, size_t count)
> +{
[ ... ]
> +	mutex_lock(&data->update_lock);
> +	data->config_fan = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);

[Severity: High]
Does this code handle I2C read failures correctly?

If i2c_smbus_read_byte_data() fails, it returns a negative error code.
Because data->config_fan is an 8-bit unsigned variable, this negative
error code is silently truncated into a large positive value.

This corrupted value is then modified and unconditionally written back to
LM63_REG_CONFIG_FAN below, which could clobber other configuration bits
in the register and potentially break hardware fan control.

> +	if (slow_clock)
> +		data->config_fan |= 0x08;
> +	else
> +		data->config_fan &= ~0x08;
> +	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
> +	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
> +	data->pwm1_freq = pfr;
> +
> +	if (data->kind == lm96163) {
> +		u8 enh = i2c_smbus_read_byte_data(client,
> +						  LM96163_REG_CONFIG_ENHANCED);

[Severity: High]
Can this read also fail and silently truncate a negative error code to an
8-bit unsigned variable?

> +		data->pwm_highres = !slow_clock && pfr == 8 && (enh & 0x10);
> +	}
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522135421.2951534-1-kernel@jhbruhn.de?part=1