| Message ID | 20240701212348.1670617-3-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (amc6821) Various improvements | expand |
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@roeck-us.net> > --- > 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. Nitpicking though, therefore: Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
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@roeck-us.net> >> --- >> 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
Hi Guenter, On 7/3/24 11:48 PM, Guenter Roeck wrote: > 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@roeck-us.net> >>> --- >>> 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 ? > No, I misread the code and I didn't see the clamp_val, which means we cannot have the denominator be > 6000000, meaning val cannot be 0 after that line (well, except if it is 0 **before** already). So no, just brain fart. Also, we probably could swap clamp_val(val, 1, 6000000) for min(val, 6000000) as val > 0 because of the ternary operator condition. But that's nothing important nor interesting. Cheers, Quentin
On 7/4/24 00:52, Quentin Schulz wrote: > Hi Guenter, > > On 7/3/24 11:48 PM, Guenter Roeck wrote: >> 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@roeck-us.net> >>>> --- >>>> 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 ? >> > > No, I misread the code and I didn't see the clamp_val, which means we cannot have the denominator be > 6000000, meaning val cannot be 0 after that line (well, except if it is 0 **before** already). > > So no, just brain fart. > > Also, we probably could swap clamp_val(val, 1, 6000000) for min(val, 6000000) as val > 0 because of the ternary operator condition. But that's nothing important nor interesting. > Good point. I may do that if I have to send v4 (if I remember), but for now the existing code should be good enough. Thanks, Guenter
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; mutex_lock(&data->update_lock); - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); + data->fan[ix] = clamp_val(val, 0, 0xFFFF); if (i2c_smbus_write_byte_data(client, fan_reg_low[ix], data->fan[ix] & 0xFF)) { dev_err(&client->dev, "Register write error, aborting.\n");
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@roeck-us.net> --- 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(-)