[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <56ed417c-1dc2-474b-a2bc-5f17e1d2bd60@cherry.de>
Date: Thu, 4 Jul 2024 09:52:05 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan
speed limits consistent
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@...ck-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
Powered by blists - more mailing lists