[lm-sensors] [PATCH] adm1026: fix setting fan_div (2nd try)

[lm-sensors] [PATCH] adm1026: fix setting fan_div (2nd try)

[Gabriele Gorla]

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

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-30 16:35:27.000000000 -0800
@@ -918,34 +918,36 @@
 {
 	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 (orig_div != DIV_FROM_REG(new_div)) {
+		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 (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));
+		}
 
-	if (data->fan_div[nr] != orig_div) {
 		fixup_fan_min(dev, nr, orig_div);
 	}
 	mutex_unlock(&data->update_lock);
 	return count;
 }

_______________________________________________
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 (2nd try)

[Phil Pokorny]

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

ACK

On Tue, Nov 30, 2010 at 4:57 PM, Gabriele Gorla <gorlik@penguintown.net>wrote:

> Prevent setting fan_div from stomping on other fans that share the same i2c
> register.
>
> 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-30
> 16:35:27.000000000 -0800
> @@ -918,34 +918,36 @@
>  {
>        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 (orig_div != DIV_FROM_REG(new_div)) {
> +               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 (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));
> +               }
>
> -       if (data->fan_div[nr] != orig_div) {
>                fixup_fan_min(dev, nr, orig_div);
>        }
>        mutex_unlock(&data->update_lock);
>        return count;
>  }
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>

[-- Attachment #1.2: Type: text/html, Size: 3898 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 (2nd try)

[Jean Delvare]

Hi Gabriele,

On Tue, 30 Nov 2010 16:57:17 -0800, Gabriele Gorla wrote:
> Prevent setting fan_div from stomping on other fans that share the same i2c register.
> 
> 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-30 16:35:27.000000000 -0800
> @@ -918,34 +918,36 @@
>  {
>  	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 (orig_div != DIV_FROM_REG(new_div)) {
> +		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 (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));
> +		}
>  
> -	if (data->fan_div[nr] != orig_div) {
>  		fixup_fan_min(dev, nr, orig_div);
>  	}
>  	mutex_unlock(&data->update_lock);
>  	return count;
>  }

Your patch is correct. However, in order to get the fix in stable
kernel series, I would prefer an even more minimalist fix, only
addressing the actual bug. The two minor optimizations (skipping the
register write if the value is the same and skipping the call to
fixup_fan_min if the value is different but resulted in the same
register encoding) would rather be left out at this time.

This would result in the following patch:

---
 drivers/hwmon/adm1026.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

--- linux-2.6.37-rc4.orig/drivers/hwmon/adm1026.c	2010-12-01 13:48:13.000000000 +0100
+++ linux-2.6.37-rc4/drivers/hwmon/adm1026.c	2010-12-01 14:20:14.000000000 +0100
@@ -916,7 +916,7 @@ static ssize_t set_fan_div(struct device
 	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);
@@ -928,15 +928,17 @@ static ssize_t set_fan_div(struct device
 	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)));
+				    (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 */
-		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)));
+				    (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));
 	}
 
 	if (data->fan_div[nr] != orig_div) {

As you can see, it's more compact and immediately obviously correct. So
backporting it to kernels 2.6.36, 2.6.32, 2.6.27 and maybe even
2.6.16 will be trivial. Is this OK with you? You retain patch
authorship, of course.

Regarding the optimizations, I'm not even sure if we need them.

The first optimization (skipping the register write if the value is the
same) looks nice but OTOH it is not something hwmon drivers are
supposed to do. Drivers should do what the user tells them, so if the
user says "write divider value 4 to the chip", it should just do so. If
you look at all other similar functions in the driver (e.g.
set_fan_min) you'll see it doesn't do any test and unconditionally
writes the value. As Guenter underlined before, "set" operations are
rare enough that there is little point in optimizing them.

As for the second optimization (skipping the call to fixup_fan_min if
the value is different but resulted in the same register encoding), if
we do strict checking of input values, it will become a no-op because
the cases it optimizes will no longer possibly happen.

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 (2nd try)

[Gabriele Gorla]

On Wed, Dec 01, 2010 at 02:59:06PM +0100, Jean Delvare wrote:

> Your patch is correct. However, in order to get the fix in stable
> kernel series, I would prefer an even more minimalist fix, only
> addressing the actual bug. The two minor optimizations (skipping the
> register write if the value is the same and skipping the call to
> fixup_fan_min if the value is different but resulted in the same
> register encoding) would rather be left out at this time.
> 
> This would result in the following patch:
> 
> ---
>  drivers/hwmon/adm1026.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> --- linux-2.6.37-rc4.orig/drivers/hwmon/adm1026.c	2010-12-01 13:48:13.000000000 +0100
> +++ linux-2.6.37-rc4/drivers/hwmon/adm1026.c	2010-12-01 14:20:14.000000000 +0100
> @@ -916,7 +916,7 @@ static ssize_t set_fan_div(struct device
>  	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);
> @@ -928,15 +928,17 @@ static ssize_t set_fan_div(struct device
>  	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)));
> +				    (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 */
> -		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)));
> +				    (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));
>  	}
>  
>  	if (data->fan_div[nr] != orig_div) {
> 
> As you can see, it's more compact and immediately obviously correct. So
> backporting it to kernels 2.6.36, 2.6.32, 2.6.27 and maybe even
> 2.6.16 will be trivial. Is this OK with you? You retain patch
> authorship, of course.

Sounds good to me. Do I need to submit it again, or you will take care 
of the re-submission?

> Regarding the optimizations, I'm not even sure if we need them.

We could debate this issues for weeks but I am sure we all have better 
things to do than trying to overoptimize a non critical code path. :-)


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 (2nd try)

[Jean Delvare]

Hi Gabriele,

On Wed, 1 Dec 2010 18:02:31 -0800, Gabriele Gorla wrote:
> On Wed, Dec 01, 2010 at 02:59:06PM +0100, Jean Delvare wrote:
> 
> > Your patch is correct. However, in order to get the fix in stable
> > kernel series, I would prefer an even more minimalist fix, only
> > addressing the actual bug. The two minor optimizations (skipping the
> > register write if the value is the same and skipping the call to
> > fixup_fan_min if the value is different but resulted in the same
> > register encoding) would rather be left out at this time.
> > 
> > This would result in the following patch:
> > 
> > ---
> >  drivers/hwmon/adm1026.c |   16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > --- linux-2.6.37-rc4.orig/drivers/hwmon/adm1026.c	2010-12-01 13:48:13.000000000 +0100
> > +++ linux-2.6.37-rc4/drivers/hwmon/adm1026.c	2010-12-01 14:20:14.000000000 +0100
> > @@ -916,7 +916,7 @@ static ssize_t set_fan_div(struct device
> >  	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);
> > @@ -928,15 +928,17 @@ static ssize_t set_fan_div(struct device
> >  	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)));
> > +				    (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 */
> > -		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)));
> > +				    (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));
> >  	}
> >  
> >  	if (data->fan_div[nr] != orig_div) {
> > 
> > As you can see, it's more compact and immediately obviously correct. So
> > backporting it to kernels 2.6.36, 2.6.32, 2.6.27 and maybe even
> > 2.6.16 will be trivial. Is this OK with you? You retain patch
> > authorship, of course.
> 
> Sounds good to me. Do I need to submit it again, or you will take care 
> of the re-submission?

I've done it:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/hwmon-adm1026-01-fix-setting-fan_div.patch
> 
> > Regarding the optimizations, I'm not even sure if we need them.
> 
> We could debate this issues for weeks but I am sure we all have better 
> things to do than trying to overoptimize a non critical code path. :-)

Correct.

-- 
Jean Delvare

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