[lm-sensors] [PATCH] adm1026: fix setting fan_div

[lm-sensors] [PATCH] adm1026: fix setting fan_div

[Gabriele Gorla]

Prevent setting fan_div from stomping on other fans that share the same i2c register. 
Also allow div=1 (this is allowed in the ADM1026 datasheet) and prevent the mutex lock 
when no update is necessary.

Signed-off-by: Gabriele Gorla <gorlik at penguintown.net>
---


diff -r -U 5 linux-source-2.6.26_orig/drivers/hwmon/adm1026.c linux-source-2.6.26/drivers/hwmon/adm1026.c
--- linux-source-2.6.26_orig/drivers/hwmon/adm1026.c	2008-07-13 14:51:29.000000000 -0700
+++ linux-source-2.6.26/drivers/hwmon/adm1026.c	2010-11-29 17:21:06.000000000 -0800
@@ -918,37 +918,42 @@
 {
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
 	int nr = sensor_attr->index;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adm1026_data *data = i2c_get_clientdata(client);
-	int val, orig_div, new_div, shift;
+	int val, orig_div, new_div;
 
 	val = simple_strtol(buf, NULL, 10);
-	new_div = DIV_TO_REG(val);
-	if (new_div = 0) {
-		return -EINVAL;
-	}
-	mutex_lock(&data->update_lock);
-	orig_div = data->fan_div[nr];
-	data->fan_div[nr] = DIV_FROM_REG(new_div);
 
-	if (nr < 4) { /* 0 <= nr < 4 */
-		shift = 2 * nr;
-		adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
-			((DIV_TO_REG(orig_div) & (~(0x03 << shift))) |
-			(new_div << shift)));
-	} else { /* 3 < nr < 8 */
-		shift = 2 * (nr - 4);
-		adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
-			((DIV_TO_REG(orig_div) & (~(0x03 << (2 * shift)))) |
-			(new_div << shift)));
+	if(val<1 || val>8) {
+	        return -EINVAL;
 	}
 
-	if (data->fan_div[nr] != orig_div) {
+	new_div = DIV_TO_REG(val);
+	if(data->fan_div[nr]!=DIV_FROM_REG(new_div)) {
+
+	        mutex_lock(&data->update_lock);
+		orig_div = data->fan_div[nr];
+		data->fan_div[nr] = DIV_FROM_REG(new_div);
+
+		if (nr < 4) { /* 0 <= nr < 4 */
+		        adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
+				    (DIV_TO_REG(data->fan_div[0]) << 0) |
+				    (DIV_TO_REG(data->fan_div[1]) << 2) |
+				    (DIV_TO_REG(data->fan_div[2]) << 4) |
+				    (DIV_TO_REG(data->fan_div[3]) << 6) );
+		} else { /* 3 < nr < 8 */
+		        adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
+				    (DIV_TO_REG(data->fan_div[4]) << 0) |
+				    (DIV_TO_REG(data->fan_div[5]) << 2) |
+				    (DIV_TO_REG(data->fan_div[6]) << 4) |
+				    (DIV_TO_REG(data->fan_div[7]) << 6) );
+		}
+		
 		fixup_fan_min(dev, nr, orig_div);
+		mutex_unlock(&data->update_lock);
 	}
-	mutex_unlock(&data->update_lock);
 	return count;
 }
 
 #define fan_offset_div(offset)						\
 static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR,		\

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Phil Pokorny]

[-- Attachment #1.1: Type: text/plain, Size: 1362 bytes --]

Thanks for the patch.

On Mon, Nov 29, 2010 at 5:49 PM, Gabriele Gorla <gorlik@penguintown.net>wrote:

> Prevent setting fan_div from stomping on other fans that share the same i2c
> register.
>

Ugh...  Wow, that was clearly wrong logic now that you point it out...


> Also allow div=1 (this is allowed in the ADM1026 datasheet)


The old code didn't throw an EINVAL on most invalid divisors, it just
silently converted them to the next reasonable value in the way that
DIV_TO_REG worked.  If you're going to change the behavior and throw EINVAL
for divisors greater than 8, perhaps you should check explicilty for the
four legal values (val != 1 && val != 2 && val != 4 && val != 8)

Then you could eliminate the new_div DIV_TO_REG/REG_TO_DIV conversions and
just test fan_div[nr] against val.  That would eliminate the "new_div"
variable which would be good since it doesn't actually hold a "div" but a
"reg" value, so it's somewhat mis-named.


> and prevent the mutex lock
> when no update is necessary.
>
>
If you don't take the mutex before testing the fan_div, then isn't there a
possible race where you test first, but then it changes before you take the
mutex to "change" it?  Is there any possible harm?  If multiple threads are
trying to set the same divisor or different divisors for the same or
different fans?  Just thinking out loud...

Phil P.

[-- Attachment #1.2: Type: text/html, Size: 2068 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Gabriele Gorla]

On Mon, Nov 29, 2010 at 06:29:08PM -0800, Phil Pokorny wrote:
> Thanks for the patch.
> 
> On Mon, Nov 29, 2010 at 5:49 PM, Gabriele Gorla <gorlik@penguintown.net>wrote:
> 
> > Prevent setting fan_div from stomping on other fans that share the same i2c
> > register.
> >
> 
> Ugh...  Wow, that was clearly wrong logic now that you point it out...

:-)


> > Also allow div=1 (this is allowed in the ADM1026 datasheet)
> 
> 
> The old code didn't throw an EINVAL on most invalid divisors, it just
> silently converted them to the next reasonable value in the way that
> DIV_TO_REG worked.  If you're going to change the behavior and throw EINVAL
> for divisors greater than 8, perhaps you should check explicilty for the
> four legal values (val != 1 && val != 2 && val != 4 && val != 8)

the original code would refuse setting div to 1 since DIV_TO_REG(1) = 0
I changed to return -EINVAL for values not in the 1-8 range.
Anything in between gets the old behavior. If you think I should change to 
allow only 1,2,4,8 I can send another patch.

> Then you could eliminate the new_div DIV_TO_REG/REG_TO_DIV conversions and
> just test fan_div[nr] against val.  That would eliminate the "new_div"
> variable which would be good since it doesn't actually hold a "div" but a
> "reg" value, so it's somewhat mis-named.


> > and prevent the mutex lock
> > when no update is necessary.
> >
> If you don't take the mutex before testing the fan_div, then isn't there a
> possible race where you test first, but then it changes before you take the
> mutex to "change" it?  Is there any possible harm?  If multiple threads are
> trying to set the same divisor or different divisors for the same or
> different fans?  Just thinking out loud...

updates to data->fandiv[x] are atomic as there are no read-modify-writes to the 
structure members. If the values changes between the test and the lock, it will 
just be replaced with a new value.
The lock will take care of multiple threads trying to change div values for different 
fans. 
If multiple threads change the div to a different value for the same fan, I think, 
even with the lock around the entire function, it will not be possible to figure out 
which value will end up in the register at the end.
Let me know what you think.

thanks,
GG

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Jean Delvare]

Hi Gabriele,

On Mon, 29 Nov 2010 17:49:38 -0800, Gabriele Gorla wrote:
> Prevent setting fan_div from stomping on other fans that share the same i2c register. 
> Also allow div=1 (this is allowed in the ADM1026 datasheet) and prevent the mutex lock 
> when no update is necessary.
> 
> Signed-off-by: Gabriele Gorla <gorlik at penguintown.net>

Thanks for the patch. It's hard to believe the code has been broken for
so long and nobody complained...

Please run your patch through scripts/checkpatch.pl, and fix all the
style errors before you resubmit.

> ---
> 
> 
> diff -r -U 5 linux-source-2.6.26_orig/drivers/hwmon/adm1026.c linux-source-2.6.26/drivers/hwmon/adm1026.c
> --- linux-source-2.6.26_orig/drivers/hwmon/adm1026.c	2008-07-13 14:51:29.000000000 -0700
> +++ linux-source-2.6.26/drivers/hwmon/adm1026.c	2010-11-29 17:21:06.000000000 -0800
> @@ -918,37 +918,42 @@
>  {
>  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>  	int nr = sensor_attr->index;
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct adm1026_data *data = i2c_get_clientdata(client);
> -	int val, orig_div, new_div, shift;
> +	int val, orig_div, new_div;
>  
>  	val = simple_strtol(buf, NULL, 10);
> -	new_div = DIV_TO_REG(val);
> -	if (new_div = 0) {
> -		return -EINVAL;
> -	}
> -	mutex_lock(&data->update_lock);
> -	orig_div = data->fan_div[nr];
> -	data->fan_div[nr] = DIV_FROM_REG(new_div);
>  
> -	if (nr < 4) { /* 0 <= nr < 4 */
> -		shift = 2 * nr;
> -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> -			((DIV_TO_REG(orig_div) & (~(0x03 << shift))) |
> -			(new_div << shift)));
> -	} else { /* 3 < nr < 8 */
> -		shift = 2 * (nr - 4);
> -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> -			((DIV_TO_REG(orig_div) & (~(0x03 << (2 * shift)))) |
> -			(new_div << shift)));
> +	if(val<1 || val>8) {
> +	        return -EINVAL;

As underlined by Phil already, this is a little inconsistent. You
should reject all invalid values, as documented at the end of
Documentation/hwmon/sysfs-interface. But this should be done in a
separate patch, as this isn't really a bug fix and changes the driver's
behavior on invalid input.

>  	}
>  
> -	if (data->fan_div[nr] != orig_div) {
> +	new_div = DIV_TO_REG(val);
> +	if(data->fan_div[nr]!=DIV_FROM_REG(new_div)) {
> +

Undue blank line.

> +	        mutex_lock(&data->update_lock);
> +		orig_div = data->fan_div[nr];
> +		data->fan_div[nr] = DIV_FROM_REG(new_div);
> +
> +		if (nr < 4) { /* 0 <= nr < 4 */
> +		        adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> +				    (DIV_TO_REG(data->fan_div[0]) << 0) |
> +				    (DIV_TO_REG(data->fan_div[1]) << 2) |
> +				    (DIV_TO_REG(data->fan_div[2]) << 4) |
> +				    (DIV_TO_REG(data->fan_div[3]) << 6) );
> +		} else { /* 3 < nr < 8 */
> +		        adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> +				    (DIV_TO_REG(data->fan_div[4]) << 0) |
> +				    (DIV_TO_REG(data->fan_div[5]) << 2) |
> +				    (DIV_TO_REG(data->fan_div[6]) << 4) |
> +				    (DIV_TO_REG(data->fan_div[7]) << 6) );
> +		}

Note: this is horribly inefficient. In my opinion, data->fan_div should
hold split but un-decoded register values. Calling DIV_FROM_REG() is
cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
shouldn't exist in the first place, as the conversion should happen in
a single place. Again, this is material for a separate patch.

> +		
>  		fixup_fan_min(dev, nr, orig_div);
> +		mutex_unlock(&data->update_lock);
>  	}
> -	mutex_unlock(&data->update_lock);
>  	return count;
>  }
>  
>  #define fan_offset_div(offset)						\
>  static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR,		\

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Guenter Roeck]

On Tue, Nov 30, 2010 at 03:04:59AM -0500, Gabriele Gorla wrote:
> On Mon, Nov 29, 2010 at 06:29:08PM -0800, Phil Pokorny wrote:
> > Thanks for the patch.
> > 
> > On Mon, Nov 29, 2010 at 5:49 PM, Gabriele Gorla <gorlik@penguintown.net>wrote:
> > 
> > > Prevent setting fan_div from stomping on other fans that share the same i2c
> > > register.
> > >
> > 
> > Ugh...  Wow, that was clearly wrong logic now that you point it out...
> 
> :-)
> 
> 
> > > Also allow div=1 (this is allowed in the ADM1026 datasheet)
> > 
> > 
> > The old code didn't throw an EINVAL on most invalid divisors, it just
> > silently converted them to the next reasonable value in the way that
> > DIV_TO_REG worked.  If you're going to change the behavior and throw EINVAL
> > for divisors greater than 8, perhaps you should check explicilty for the
> > four legal values (val != 1 && val != 2 && val != 4 && val != 8)
> 
> the original code would refuse setting div to 1 since DIV_TO_REG(1) = 0
> I changed to return -EINVAL for values not in the 1-8 range.
> Anything in between gets the old behavior. If you think I should change to 
> allow only 1,2,4,8 I can send another patch.
> 
> > Then you could eliminate the new_div DIV_TO_REG/REG_TO_DIV conversions and
> > just test fan_div[nr] against val.  That would eliminate the "new_div"
> > variable which would be good since it doesn't actually hold a "div" but a
> > "reg" value, so it's somewhat mis-named.
> 
> 
> > > and prevent the mutex lock
> > > when no update is necessary.
> > >
> > If you don't take the mutex before testing the fan_div, then isn't there a
> > possible race where you test first, but then it changes before you take the
> > mutex to "change" it?  Is there any possible harm?  If multiple threads are
> > trying to set the same divisor or different divisors for the same or
> > different fans?  Just thinking out loud...
> 
> updates to data->fandiv[x] are atomic as there are no read-modify-writes to the 
> structure members. If the values changes between the test and the lock, it will 
> just be replaced with a new value.
> The lock will take care of multiple threads trying to change div values for different 
> fans. 


> If multiple threads change the div to a different value for the same fan, I think, 
> even with the lock around the entire function, it will not be possible to figure out 
> which value will end up in the register at the end.

The value is well defined: The last thread to acquire the lock wins.

> Let me know what you think.
> 
Problem occurs if the 1st access (the one holding the lock) changes the value for
a fan and the second tries to change it back to the old value. As a result of your code,
the change will happen, but the revert to the old value may be ignored (if fan_div was
not yet updated by the time the if() statement outside the lock is executed).
This is not ok, since it changes execution sequence. The if statement should be inside
the lock.

The argument about unnecessary locking is really irrelevant; this code is not executed often
enough to warrant introducing a race condition.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Phil Pokorny]

[-- Attachment #1.1: Type: text/plain, Size: 2092 bytes --]

On Tue, Nov 30, 2010 at 1:17 AM, Jean Delvare <khali@linux-fr.org> wrote:

>
> > +             mutex_lock(&data->update_lock);
> > +             orig_div = data->fan_div[nr];
> > +             data->fan_div[nr] = DIV_FROM_REG(new_div);
> > +
> > +             if (nr < 4) { /* 0 <= nr < 4 */
> > +                     adm1026_write_value(client,
> ADM1026_REG_FAN_DIV_0_3,
> > +                                 (DIV_TO_REG(data->fan_div[0]) << 0) |
> > +                                 (DIV_TO_REG(data->fan_div[1]) << 2) |
> > +                                 (DIV_TO_REG(data->fan_div[2]) << 4) |
> > +                                 (DIV_TO_REG(data->fan_div[3]) << 6) );
> > +             } else { /* 3 < nr < 8 */
> > +                     adm1026_write_value(client,
> ADM1026_REG_FAN_DIV_4_7,
> > +                                 (DIV_TO_REG(data->fan_div[4]) << 0) |
> > +                                 (DIV_TO_REG(data->fan_div[5]) << 2) |
> > +                                 (DIV_TO_REG(data->fan_div[6]) << 4) |
> > +                                 (DIV_TO_REG(data->fan_div[7]) << 6) );
> > +             }
>
>
> Note: this is horribly inefficient. In my opinion, data->fan_div should
> hold split but un-decoded register values. Calling DIV_FROM_REG() is
> cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> shouldn't exist in the first place, as the conversion should happen in
> a single place. Again, this is material for a separate patch.
>
>
I humbly suggest that you are forgetting something.  With your
recommendation, the REG_TO_DIV function would be called many orders of
magnitute more times (thousands? millions?) than the DIV_TO_REG function
here.  REG_TO_DIV would have to be called every time the fan speed is _read_
but DIV_TO_REG only needs to be called the one or two times that the fan_div
is _written_  So while the macros might differ in complexity, the number of
times they are called swamps any economy gained by changing the fan_div from
divisor to register value.

The code above is clear in and appropriate in my opinion.

Phil P.

[-- Attachment #1.2: Type: text/html, Size: 2624 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Gabriele Gorla]

On Tue, Nov 30, 2010 at 10:17:13AM +0100, Jean Delvare wrote:
> Thanks for the patch. It's hard to believe the code has been broken for
> so long and nobody complained...
> 
> Please run your patch through scripts/checkpatch.pl, and fix all the
> style errors before you resubmit.

ok, I will.

> > 
> > diff -r -U 5 linux-source-2.6.26_orig/drivers/hwmon/adm1026.c linux-source-2.6.26/drivers/hwmon/adm1026.c
> > --- linux-source-2.6.26_orig/drivers/hwmon/adm1026.c	2008-07-13 14:51:29.000000000 -0700
> > +++ linux-source-2.6.26/drivers/hwmon/adm1026.c	2010-11-29 17:21:06.000000000 -0800
> > @@ -918,37 +918,42 @@
> >  {
> >  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> >  	int nr = sensor_attr->index;
> >  	struct i2c_client *client = to_i2c_client(dev);
> >  	struct adm1026_data *data = i2c_get_clientdata(client);
> > -	int val, orig_div, new_div, shift;
> > +	int val, orig_div, new_div;
> >  
> >  	val = simple_strtol(buf, NULL, 10);
> > -	new_div = DIV_TO_REG(val);
> > -	if (new_div = 0) {
> > -		return -EINVAL;
> > -	}
> > -	mutex_lock(&data->update_lock);
> > -	orig_div = data->fan_div[nr];
> > -	data->fan_div[nr] = DIV_FROM_REG(new_div);
> >  
> > -	if (nr < 4) { /* 0 <= nr < 4 */
> > -		shift = 2 * nr;
> > -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> > -			((DIV_TO_REG(orig_div) & (~(0x03 << shift))) |
> > -			(new_div << shift)));
> > -	} else { /* 3 < nr < 8 */
> > -		shift = 2 * (nr - 4);
> > -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > -			((DIV_TO_REG(orig_div) & (~(0x03 << (2 * shift)))) |
> > -			(new_div << shift)));
> > +	if(val<1 || val>8) {
> > +	        return -EINVAL;
> 
> As underlined by Phil already, this is a little inconsistent. You
> should reject all invalid values, as documented at the end of
> Documentation/hwmon/sysfs-interface. But this should be done in a
> separate patch, as this isn't really a bug fix and changes the driver's
> behavior on invalid input.

Original code will reject 1 as input. I think this is a bug as div=1 is
valid for adm1026.
Anyway, if you think I should separate the patch and reject all invalid
values it's ok for me.

> > +		        adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > +				    (DIV_TO_REG(data->fan_div[4]) << 0) |
> > +				    (DIV_TO_REG(data->fan_div[5]) << 2) |
> > +				    (DIV_TO_REG(data->fan_div[6]) << 4) |
> > +				    (DIV_TO_REG(data->fan_div[7]) << 6) );
> > +		}
> 
> Note: this is horribly inefficient. In my opinion, data->fan_div should
> hold split but un-decoded register values. Calling DIV_FROM_REG() is
> cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> shouldn't exist in the first place, as the conversion should happen in
> a single place. Again, this is material for a separate patch.

agree.

thanks,
GG

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Phil Pokorny]

[-- Attachment #1.1: Type: text/plain, Size: 1721 bytes --]

On Tue, Nov 30, 2010 at 11:10 AM, Gabriele Gorla <gorlik@penguintown.net>wrote:

> On Tue, Nov 30, 2010 at 10:17:13AM +0100, Jean Delvare wrote:
> > > +   if(val<1 || val>8) {
> > > +           return -EINVAL;
> >
> > As underlined by Phil already, this is a little inconsistent. You
> > should reject all invalid values, as documented at the end of
> > Documentation/hwmon/sysfs-interface. But this should be done in a
> > separate patch, as this isn't really a bug fix and changes the driver's
> > behavior on invalid input.
>
> Original code will reject 1 as input. I think this is a bug as div=1 is
> valid for adm1026.
> Anyway, if you think I should separate the patch and reject all invalid
> values it's ok for me.
>
>
Agree.  Thanks


> > +                   adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > > +                               (DIV_TO_REG(data->fan_div[4]) << 0) |
> > > +                               (DIV_TO_REG(data->fan_div[5]) << 2) |
> > > +                               (DIV_TO_REG(data->fan_div[6]) << 4) |
> > > +                               (DIV_TO_REG(data->fan_div[7]) << 6) );
> > > +           }
> >
> > Note: this is horribly inefficient. In my opinion, data->fan_div should
> > hold split but un-decoded register values. Calling DIV_FROM_REG() is
> > cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> > shouldn't exist in the first place, as the conversion should happen in
> > a single place. Again, this is material for a separate patch.
>
> agree.
>
>
NACK.

I disagree as I said before.  fan speed reading code gets called many more
times than the fan_div setting code and there is no savings in changing
fan_div from "div" to "reg" values.

Phil P.

[-- Attachment #1.2: Type: text/html, Size: 2538 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Jean Delvare]

Hi Gabriele,

On Tue, 30 Nov 2010 11:10:53 -0800, Gabriele Gorla wrote:
> On Tue, Nov 30, 2010 at 10:17:13AM +0100, Jean Delvare wrote:
> > > diff -r -U 5 linux-source-2.6.26_orig/drivers/hwmon/adm1026.c linux-source-2.6.26/drivers/hwmon/adm1026.c
> > > --- linux-source-2.6.26_orig/drivers/hwmon/adm1026.c	2008-07-13 14:51:29.000000000 -0700
> > > +++ linux-source-2.6.26/drivers/hwmon/adm1026.c	2010-11-29 17:21:06.000000000 -0800
> > > @@ -918,37 +918,42 @@
> > >  {
> > >  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> > >  	int nr = sensor_attr->index;
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > >  	struct adm1026_data *data = i2c_get_clientdata(client);
> > > -	int val, orig_div, new_div, shift;
> > > +	int val, orig_div, new_div;
> > >  
> > >  	val = simple_strtol(buf, NULL, 10);
> > > -	new_div = DIV_TO_REG(val);
> > > -	if (new_div = 0) {
> > > -		return -EINVAL;
> > > -	}
> > > -	mutex_lock(&data->update_lock);
> > > -	orig_div = data->fan_div[nr];
> > > -	data->fan_div[nr] = DIV_FROM_REG(new_div);
> > >  
> > > -	if (nr < 4) { /* 0 <= nr < 4 */
> > > -		shift = 2 * nr;
> > > -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> > > -			((DIV_TO_REG(orig_div) & (~(0x03 << shift))) |
> > > -			(new_div << shift)));
> > > -	} else { /* 3 < nr < 8 */
> > > -		shift = 2 * (nr - 4);
> > > -		adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > > -			((DIV_TO_REG(orig_div) & (~(0x03 << (2 * shift)))) |
> > > -			(new_div << shift)));
> > > +	if(val<1 || val>8) {
> > > +	        return -EINVAL;
> > 
> > As underlined by Phil already, this is a little inconsistent. You
> > should reject all invalid values, as documented at the end of
> > Documentation/hwmon/sysfs-interface. But this should be done in a
> > separate patch, as this isn't really a bug fix and changes the driver's
> > behavior on invalid input.
> 
> Original code will reject 1 as input. I think this is a bug as div=1 is
> valid for adm1026.

Yes, you are correct, and this has to be fixed, I don't disagree. But
you are also changing the behavior when the value is more than 8...

> Anyway, if you think I should separate the patch and reject all invalid
> values it's ok for me.

... and this proposed change would also change the behavior for values
3, 5, 6 and 7. We can't really do that in a stable kernel series.

Quick summary to avoid any further conclusion:

* The DIV_TO_REG() macro as it currently exists makes sense. It always
  return a valid divisor value, regardless of the input being "correct"
  or not. This goes against the recommendations of
  Documentation/hwmon/sysfs-interface (which were written long after
  the adm1026 driver was written) but this isn't a big issue per se.

* Ergo, the current validity check on new_div is a plain bug. There's
  no point in checking a value which is correct by construction - even
  more so if the check itself is wrong. So the check should be removed
  as a bug fix in stable kernel series.

* The new validity check you propose is correct, but doesn't comply
  with Documentation/hwmon/sysfs-interface either. If we're changing
  the behavior, we should at least follow the standard. This is why I
  (and Phil) suggest that you change your test.

I hope it's all clear now :)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Jean Delvare]

Hi Phil,

On Tue, 30 Nov 2010 11:05:22 -0800, Phil Pokorny wrote:
> On Tue, Nov 30, 2010 at 1:17 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > +             mutex_lock(&data->update_lock);
> > > +             orig_div = data->fan_div[nr];
> > > +             data->fan_div[nr] = DIV_FROM_REG(new_div);
> > > +
> > > +             if (nr < 4) { /* 0 <= nr < 4 */
> > > +                     adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> > > +                                 (DIV_TO_REG(data->fan_div[0]) << 0) |
> > > +                                 (DIV_TO_REG(data->fan_div[1]) << 2) |
> > > +                                 (DIV_TO_REG(data->fan_div[2]) << 4) |
> > > +                                 (DIV_TO_REG(data->fan_div[3]) << 6) );
> > > +             } else { /* 3 < nr < 8 */
> > > +                     adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > > +                                 (DIV_TO_REG(data->fan_div[4]) << 0) |
> > > +                                 (DIV_TO_REG(data->fan_div[5]) << 2) |
> > > +                                 (DIV_TO_REG(data->fan_div[6]) << 4) |
> > > +                                 (DIV_TO_REG(data->fan_div[7]) << 6) );
> > > +             }
> >
> > Note: this is horribly inefficient. In my opinion, data->fan_div should
> > hold split but un-decoded register values. Calling DIV_FROM_REG() is
> > cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> > shouldn't exist in the first place, as the conversion should happen in
> > a single place. Again, this is material for a separate patch.
>
> I humbly suggest that you are forgetting something.  With your
> recommendation, the REG_TO_DIV function would be called many orders of
> magnitute more times (thousands? millions?) than the DIV_TO_REG function
> here.  REG_TO_DIV would have to be called every time the fan speed is _read_
> but DIV_TO_REG only needs to be called the one or two times that the fan_div
> is _written_  So while the macros might differ in complexity, the number of
> times they are called swamps any economy gained by changing the fan_div from
> divisor to register value.

You are right on one thing. In all honesty, I had forgotten that
DIV_FROM_REG() would not only get called for fanX_div sysfs attributes
but also for fanX_input and fanX_min attributes, which are obviously
called repeatedly by monitoring applications, so their performance
matters.

And I agree with you that calling DIV_FROM_REG() in show_fan() and
show_fan_min() - or even directly in FAN_FROM_REG() - would be costly.

However, we are allowed to be smart (at least I hope so!) Converting
the divisor register value (0 to 3) into an actual scaling factor (1,
2, 4 or 8) to divide or multiply by is inefficient. It's much better to
use the register value directly to bit-shift the fan input value or
limit value. Concretely, this means that, if data->fan_div[nr] contains
a split register value, you don't want to do:

#define FAN_FROM_REG(val, div) ((val) = 0 ? -1 : (val) = 0xff ? 0 : \
				1350000 / ((val) * DIV_FROM_REG(div)))

but:

#define FAN_FROM_REG(val, div) ((val) = 0 ? -1 : (val) = 0xff ? 0 : \
				1350000 / ((val) << (div)))

And the same holds for FAN_TO_REG(). In the end, DIV_FROM_REG() could
only be called from show_fan_div(), and DIV_TO_REG only called once
from set_fan_div() - so we could in fact get rid of both macros.

> The code above is clear in and appropriate in my opinion.

The code above is good enough and has the merit of not being too
intrusive. But I still claim that my counter-proposal would make the code
cleaner and more efficient... in a future kernel version.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Gabriele Gorla]

Thanks everyone for the feedback.

I resubmitted 2 bugfixes only patches:
1- to fix the div setting
2- stop rejecting div=1 since it is valid

I left the locking around the entire structure access as requested.

I can submit another patch to validate the input to 1,2,4,8 to be applied in the development 
kernels.

I am not sure if we reached an agreement on the internal representation of fan_div so I 
am leaving it alone for now.

thanks,
GG

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Phil Pokorny]

[-- Attachment #1.1: Type: text/plain, Size: 5374 bytes --]

On Tue, Nov 30, 2010 at 12:30 PM, Jean Delvare <khali@linux-fr.org> wrote:

> Hi Phil,
>
> On Tue, 30 Nov 2010 11:05:22 -0800, Phil Pokorny wrote:
> > On Tue, Nov 30, 2010 at 1:17 AM, Jean Delvare <khali@linux-fr.org>
> wrote:
> > > > +             mutex_lock(&data->update_lock);
> > > > +             orig_div = data->fan_div[nr];
> > > > +             data->fan_div[nr] = DIV_FROM_REG(new_div);
> > > > +
> > > > +             if (nr < 4) { /* 0 <= nr < 4 */
> > > > +                     adm1026_write_value(client,
> ADM1026_REG_FAN_DIV_0_3,
> > > > +                                 (DIV_TO_REG(data->fan_div[0]) << 0)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[1]) << 2)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[2]) << 4)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[3]) << 6)
> );
> > > > +             } else { /* 3 < nr < 8 */
> > > > +                     adm1026_write_value(client,
> ADM1026_REG_FAN_DIV_4_7,
> > > > +                                 (DIV_TO_REG(data->fan_div[4]) << 0)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[5]) << 2)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[6]) << 4)
> |
> > > > +                                 (DIV_TO_REG(data->fan_div[7]) << 6)
> );
> > > > +             }
> > >
> > > Note: this is horribly inefficient. In my opinion, data->fan_div should
> > > hold split but un-decoded register values. Calling DIV_FROM_REG() is
> > > cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> > > shouldn't exist in the first place, as the conversion should happen in
> > > a single place. Again, this is material for a separate patch.
> >
> > I humbly suggest that you are forgetting something.  With your
> > recommendation, the REG_TO_DIV function would be called many orders of
> > magnitute more times (thousands? millions?) than the DIV_TO_REG function
> > here.  REG_TO_DIV would have to be called every time the fan speed is
> _read_
> > but DIV_TO_REG only needs to be called the one or two times that the
> fan_div
> > is _written_  So while the macros might differ in complexity, the number
> of
> > times they are called swamps any economy gained by changing the fan_div
> from
> > divisor to register value.
>
> You are right on one thing. In all honesty, I had forgotten that
> DIV_FROM_REG() would not only get called for fanX_div sysfs attributes
> but also for fanX_input and fanX_min attributes, which are obviously
> called repeatedly by monitoring applications, so their performance
> matters.
>
> And I agree with you that calling DIV_FROM_REG() in show_fan() and
> show_fan_min() - or even directly in FAN_FROM_REG() - would be costly.
>
> However, we are allowed to be smart (at least I hope so!) Converting
> the divisor register value (0 to 3) into an actual scaling factor (1,
> 2, 4 or 8) to divide or multiply by is inefficient. It's much better to
> use the register value directly to bit-shift the fan input value or
> limit value. Concretely, this means that, if data->fan_div[nr] contains
> a split register value, you don't want to do:
>
> #define FAN_FROM_REG(val, div) ((val) == 0 ? -1 : (val) == 0xff ? 0 : \
>                                1350000 / ((val) * DIV_FROM_REG(div)))
>
> but:
>
> #define FAN_FROM_REG(val, div) ((val) == 0 ? -1 : (val) == 0xff ? 0 : \
>                                1350000 / ((val) << (div)))
>
> And the same holds for FAN_TO_REG(). In the end, DIV_FROM_REG() could
> only be called from show_fan_div(), and DIV_TO_REG only called once
> from set_fan_div() - so we could in fact get rid of both macros.
>

Hmm.  That's creative...

However, the optimization comes at the expense of generality.  This
alternative only allows for power of 2 fan divisors.  This hardware only
supports powers of 2, and probably all other chips are the same.  But other
hardware might support other divisors.  Is there an advantage to having
similar or identical FAN_FROM_REG macros across multiple chips?

Also, it's not necessarily a given that shifts are faster than multiplies.
Integer multipliers can be faster than shifts if a CPU doesn't include a
barrel shifter than can perform arbitrary shifts, but does them with the
equivalent of a loop.  I think the Pentium PRO? had this issue.  It was
mentioned during the AES design bakeoffs that multi-bit shifts could be
expensive on some architectures.  Of course the first SPARC chips were
notable for _not_ having an integer multiply instruction.

In this case, it seems like a multiplication is the appropriate operator for
a "fan divisor" value.  Let the compiler choose the best code.

And your alternative is still clever.


> The code above is clear in and appropriate in my opinion.
>
> The code above is good enough and has the merit of not being too
> intrusive. But I still claim that my counter-proposal would make the code
> cleaner and more efficient... in a future kernel version.
>

I would suggest that such a patch should change the name of "fan_div" to
"fan_div_shift" or "fan_shift" since it's not a divisor.  However, I
recognize that other drivers (Winbond family...) store a "register" shift
value in fan_div...

But I think we've wandered far from the original issue of broken code that
clobbered all the fan divisors when setting any one.

Phil P.

[-- Attachment #1.2: Type: text/html, Size: 6569 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Jean Delvare]

Hi Gabriele,

On Tue, 30 Nov 2010 17:05:48 -0800, Gabriele Gorla wrote:
> Thanks everyone for the feedback.
> 
> I resubmitted 2 bugfixes only patches:
> 1- to fix the div setting
> 2- stop rejecting div=1 since it is valid
> 
> I left the locking around the entire structure access as requested.
> 
> I can submit another patch to validate the input to 1,2,4,8 to be applied in the development 
> kernels.

Yes, please. This looks like a very good plan, thank you.

> I am not sure if we reached an agreement on the internal representation of fan_div so I 
> am leaving it alone for now.

This can be done later, by you or by me (but then I'll count on you for
testing, as I assume you have an ADM1026 chip and I don't.)

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Jean Delvare]

Hi Phil,

On Tue, 30 Nov 2010 18:46:49 -0800, Phil Pokorny wrote:
> On Tue, Nov 30, 2010 at 12:30 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > You are right on one thing. In all honesty, I had forgotten that
> > DIV_FROM_REG() would not only get called for fanX_div sysfs attributes
> > but also for fanX_input and fanX_min attributes, which are obviously
> > called repeatedly by monitoring applications, so their performance
> > matters.
> >
> > And I agree with you that calling DIV_FROM_REG() in show_fan() and
> > show_fan_min() - or even directly in FAN_FROM_REG() - would be costly.
> >
> > However, we are allowed to be smart (at least I hope so!) Converting
> > the divisor register value (0 to 3) into an actual scaling factor (1,
> > 2, 4 or 8) to divide or multiply by is inefficient. It's much better to
> > use the register value directly to bit-shift the fan input value or
> > limit value. Concretely, this means that, if data->fan_div[nr] contains
> > a split register value, you don't want to do:
> >
> > #define FAN_FROM_REG(val, div) ((val) = 0 ? -1 : (val) = 0xff ? 0 : \
> >                                1350000 / ((val) * DIV_FROM_REG(div)))
> >
> > but:
> >
> > #define FAN_FROM_REG(val, div) ((val) = 0 ? -1 : (val) = 0xff ? 0 : \
> >                                1350000 / ((val) << (div)))
> >
> > And the same holds for FAN_TO_REG(). In the end, DIV_FROM_REG() could
> > only be called from show_fan_div(), and DIV_TO_REG only called once
> > from set_fan_div() - so we could in fact get rid of both macros.
> 
> Hmm.  That's creative...
> 
> However, the optimization comes at the expense of generality.  This
> alternative only allows for power of 2 fan divisors.  This hardware only
> supports powers of 2, and probably all other chips are the same.  But other
> hardware might support other divisors.  Is there an advantage to having
> similar or identical FAN_FROM_REG macros across multiple chips?

No advantage that I can see. The available divisor values and their
respective encoding is device-specific. See for example the FSC
devices which only support divisors 2, 4 and 8, or fan3_div of old
IT87xxF devices which can only be set to 2 or 8 and is encoded on a
single bit.

We could still have a common function or macro for the two most common
cases (divisors being powers of 2 starting from 1 and encoded on 2 or 3
bits), but we could have done this years ago and nobody did, so it's
not obviously needed. Furthermore, in the case of powers of 2, as I just
explained, it's as easy and presumably cheaper to manipulate the raw
register values, so the need for conversion macros vanishes.
 
> Also, it's not necessarily a given that shifts are faster than multiplies.
> Integer multipliers can be faster than shifts if a CPU doesn't include a
> barrel shifter than can perform arbitrary shifts, but does them with the
> equivalent of a loop.  I think the Pentium PRO? had this issue.  It was
> mentioned during the AES design bakeoffs that multi-bit shifts could be
> expensive on some architectures.  Of course the first SPARC chips were
> notable for _not_ having an integer multiply instruction.

Obviously we depend on the actual CPU implementation, but at least on
recent x86 CPUs (and these are the main target for generic hardware
monitoring drivers such as the adm1026 driver in my experience)
shifting is somewhat faster than multiplying and much faster than
dividing.

But this is going deeper in the details than I originally wanted. My
main point was that my approach was replacing a single operation in the
hot path by another single operation, and not by a complex function or
macro call as you could fear.

> In this case, it seems like a multiplication is the appropriate operator for
> a "fan divisor" value.  Let the compiler choose the best code.

I don't think the compiler can optimize anything for us. Only constant
power-of-2 multiplications and divisions can be converted to shifts by
the compiler. We developers know that we only deal with powers of 2,
but as these aren't constants, I can't see how the compiler would know.

> And your alternative is still clever.

Not sure if this is a compliment or criticism ;)

> > The code above is good enough and has the merit of not being too
> > intrusive. But I still claim that my counter-proposal would make the code
> > cleaner and more efficient... in a future kernel version.
> 
> I would suggest that such a patch should change the name of "fan_div" to
> "fan_div_shift" or "fan_shift" since it's not a divisor.  However, I
> recognize that other drivers (Winbond family...) store a "register" shift
> value in fan_div...

This is an implementation detail. I'd be fine with "fan_div" as a
reference to the register name. What really matters IMO is that the
contents of the data structure members is properly documented in the
structure definition.

> But I think we've wandered far from the original issue of broken code that
> clobbered all the fan divisors when setting any one.

Definitely. I'm sorry I can't resist criticizing code I feel could be
optimized ;) Fixing the actual bugs with minimal patches is way more
important at the moment, I fully agree.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors