Re: [PATCH v3] 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] Missing lock in `pwm1_freq_show` causes a data race and a potential divide-by-zero kernel panic.
--

commit 1bf46f1b90af417821722eaed56484734ef9e3ae
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
sysfs attributes. It adds a pwm1_freq attribute to select the closest
SCS + PFR combination, and exposes a chip-wide pwm1_auto_point_temp_hyst
attribute for the hysteresis amount.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..90c49c448c624 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c

[ ... ]

> @@ -455,6 +458,86 @@ 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: High]
Can this read of data->pwm1_freq race with an update, resulting in a
divide-by-zero exception?

Looking at lm63_update_device(), it acquires data->update_lock and can
temporarily set data->pwm1_freq to 0 before clamping it to 1:

lm63_update_device() {
    mutex_lock(&data->update_lock);
    ...
    data->pwm1_freq = i2c_smbus_read_byte_data(client, LM63_REG_PWM_FREQ);
    if (data->pwm1_freq == 0)
        data->pwm1_freq = 1;
    ...
}

Since pwm1_freq_show() does not hold data->update_lock, could it read
data->pwm1_freq during this transient 0 state and cause a kernel panic?

> +}
> +

[ ... ]

> @@ -629,6 +712,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));

[Severity: High]
Does accessing data->lut_temp_hyst here without holding data->update_lock
also result in a data race with concurrent updates?

> +}

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