On 7/3/24 07:35, Quentin Schulz wrote:
Hi Guenter, On 7/1/24 11:23 PM, Guenter Roeck wrote:The default value of the maximum fan speed limit register is 0, essentially translating to an unlimited fan speed. When reading the limit, a value of 0 is reported in this case. However, writing a value of 0 results in writing a value of 0xffff into the register, which is inconsistent. To solve the problem, permit writing a limit of 0 for the maximim fan speed, effectively translating to "no limit". Write 0 into the register if a limit value of 0 is written. Otherwise limit the range to <1..6000000> and write 1..0xffff into the register. This ensures that reading and writing from and to a limit register return the same value while at the same time not changing reported values when reading the speed or limits. While at it, restrict fan limit writes to non-negative numbers; writing a negative limit does not make sense and should be reported instead of being corrected. Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> --- v2: Do not accept negative fan speed values Display fan speed and speed limit as 0 if register value is 0 (instead of 6000000), as in original code. Only permit writing 0 (unlimited) for the maximum fan speed. drivers/hwmon/amc6821.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index eb2d5592a41a..9c19d4d278ec 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, { struct amc6821_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; - long val; + unsigned long val; int ix = to_sensor_dev_attr(attr)->index; - int ret = kstrtol(buf, 10, &val); + int ret = kstrtoul(buf, 10, &val); if (ret) return ret; - val = 1 > val ? 0xFFFF : 6000000/val; + + /* The minimum fan speed must not be unlimited (0) */ + if (ix == IDX_FAN1_MIN && !val) + return -EINVAL; + + val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;I'm wondering if we shouldn't check !val for min after this line instead? Otherwise we allow 6000001+RPM speeds... which is technically unlimited.
If ix == IDX_FAN1_MIN, val must be positive because of the check above. The expression "6000000 / clamp_val(val, 1, 6000000)" is therefore always positive as well because val is clamped. Its minimum result would be 6000000/6000000 = 1. The alternate case of the ternary expression would never hit because it is guaranteed that val > 0. Am I missing something ? Thanks, Guenter
