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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Error handling in `pwm1_freq_store` leaves the software cache and hardware out of sync.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `pwm1_freq_store` allows writes to read-only hardware registers.
- [Medium] `pwm1_auto_point_temp_hyst_store` writes to the LUT Hysteresis register without verifying if the LUT is unlocked.
- [Medium] Unprotected reads in `pwm1_freq_show` lead to inconsistent state calculation.
--

commit 14b83e16319e09218194853a200383c1b94b855a
Author: Jan-Henrik Bruhn <kernel@jhbruhn.de>

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

This commit adds sysfs attributes to allow configuring the PWM frequency 
and LUT hysteresis in the lm63 driver. It exposes pwm1_freq for PWM base 
frequency and a single chip-wide pwm1_auto_point_temp_hyst for hysteresis.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..b95e866ed1ee4 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
[ ... ]
> @@ -455,6 +458,83 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t pwm1_freq_show(struct device *dev,
> +			      struct device_attribute *dummy, char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +	unsigned int base = (data->config_fan & 0x08) ?
> +			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
> +
> +	return sprintf(buf, "%u\n", base / data->pwm1_freq);
> +}

[Severity: Medium]
Does lm63_update_device() release the update_lock before returning?

If so, is there a race condition here? If another thread modifying these
variables in pwm1_freq_store() runs concurrently, could this code read the
new config_fan clock base but the old pwm1_freq divider, resulting in an
incorrect frequency calculation?

> +
> +/*
> + * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
> + * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
> + * bytes are register-relative; rewrite them after a frequency change
> + * if duty cycles must be preserved.
> + */
> +static ssize_t pwm1_freq_store(struct device *dev,
> +			       struct device_attribute *dummy,
> +			       const char *buf, size_t count)
> +{
> +	struct lm63_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
> +	bool slow_clock;
> +	int ret;
> +
> +	if (!(data->config_fan & 0x20)) /* register is read-only */
> +		return -EPERM;

[Severity: Medium]
Can a concurrent update to manual mode lead to a desynchronized cache?

Since this check for manual mode is outside the data->update_lock critical
section, could another thread concurrently disable manual mode before the lock
is acquired below?

If that happens, the hardware would ignore the write to LM63_REG_PWM_FREQ,
but the driver unconditionally updates data->pwm1_freq, potentially leaving the
software cache permanently desynchronized from the hardware.

> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
[ ... ]
> +	mutex_lock(&data->update_lock);
> +	ret = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
> +	if (ret < 0) {
> +		mutex_unlock(&data->update_lock);
> +		return ret;
> +	}
> +	data->config_fan = ret;
> +	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) {
> +		ret = i2c_smbus_read_byte_data(client, LM96163_REG_CONFIG_ENHANCED);
> +		if (ret < 0) {
> +			mutex_unlock(&data->update_lock);
> +			return ret;
> +		}

[Severity: High]
If this read fails, the function aborts and returns an error code without
updating data->pwm_highres.

However, data->config_fan and data->pwm1_freq have already been updated in
the cache and written to hardware. Does returning an error here leave the
hardware state out of sync with what userspace expects, while keeping the
driver's PWM scaling formula permanently desynchronized from the hardware's
actual operating mode?

> +		data->pwm_highres = !slow_clock && pfr == 8 && (ret & 0x10);
> +	}
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}
[ ... ]
> @@ -629,6 +709,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
>  	return sprintf(buf, "%d\n", temp);
>  }
>  
> +/*
> + * The LM63 has a single hysteresis register shared by all LUT entries.
> + * Expose it as a chip-wide hysteresis amount in millidegrees; the
> + * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
> + * show the resulting absolute trip-down temperature for each entry.
> + */
> +static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
> +					      struct device_attribute *dummy,
> +					      char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));
> +}
> +
> +static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
> +					       struct device_attribute *dummy,
> +					       const char *buf, size_t count)
> +{
> +	struct lm63_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &val);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&data->update_lock);
> +	data->lut_temp_hyst = HYST_TO_REG(val);
> +	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
> +				  data->lut_temp_hyst);

[Severity: Medium]
Should this function check if the device is in manual mode before writing?

If a user attempts to write to this attribute while the device is in
automatic mode, it looks like the hardware will silently ignore the write
because the LUT is locked, but the driver updates data->lut_temp_hyst.
Does this cause the sysfs interface to reflect a false state?

> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}

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