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;
 
 	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");
-- 
2.39.2

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