Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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 ?


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




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux