* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 9:13 ` Segher Boessenkool
0 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2008-03-10 9:13 UTC (permalink / raw)
To: Roel Kluin; +Cc: benh, linuxppc-dev, lkml, Colin Leroy
> diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
> index 9587869..8ea7da2 100644
> --- a/drivers/hwmon/adt7473.c
> +++ b/drivers/hwmon/adt7473.c
> @@ -570,7 +570,7 @@ static ssize_t set_max_duty_at_crit(struct device
> *dev,
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7473_data *data = i2c_get_clientdata(client);
> int temp = simple_strtol(buf, NULL, 10);
> - temp = temp && 0xFF;
> + temp &= 0xFF;
>
> mutex_lock(&data->lock);
> data->max_duty_at_overheat = temp;
The & 0xff here is bogus anyway; temp is only ever used as an u8,
so just declare it as that, or do proper overflow/underflow checking
on it. The patch will need testing on hardware too, since it changes
behaviour (it should be a bugfix, but who knows).
Segher
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 9:13 ` Segher Boessenkool
@ 2008-03-10 9:59 ` Roel Kluin
-1 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 9:59 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Colin Leroy, Darrick J. Wong, lkml, linuxppc-dev
Segher Boessenkool wrote:
>> diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
>> index 9587869..8ea7da2 100644
>> --- a/drivers/hwmon/adt7473.c
>> +++ b/drivers/hwmon/adt7473.c
>> @@ -570,7 +570,7 @@ static ssize_t set_max_duty_at_crit(struct device
>> *dev,
>> struct i2c_client *client = to_i2c_client(dev);
>> struct adt7473_data *data = i2c_get_clientdata(client);
>> int temp = simple_strtol(buf, NULL, 10);
>> - temp = temp && 0xFF;
>> + temp &= 0xFF;
>>
>> mutex_lock(&data->lock);
>> data->max_duty_at_overheat = temp;
>
> The & 0xff here is bogus anyway; temp is only ever used as an u8,
> so just declare it as that, or do proper overflow/underflow checking
> on it. The patch will need testing on hardware too, since it changes
> behaviour (it should be a bugfix, but who knows).
Maybe someone can test this?
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..2a2de73 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10) & 0xFF;
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 9:59 ` Roel Kluin
0 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 9:59 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: benh, linuxppc-dev, lkml, Colin Leroy, Darrick J. Wong
Segher Boessenkool wrote:
>> diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
>> index 9587869..8ea7da2 100644
>> --- a/drivers/hwmon/adt7473.c
>> +++ b/drivers/hwmon/adt7473.c
>> @@ -570,7 +570,7 @@ static ssize_t set_max_duty_at_crit(struct device
>> *dev,
>> struct i2c_client *client = to_i2c_client(dev);
>> struct adt7473_data *data = i2c_get_clientdata(client);
>> int temp = simple_strtol(buf, NULL, 10);
>> - temp = temp && 0xFF;
>> + temp &= 0xFF;
>>
>> mutex_lock(&data->lock);
>> data->max_duty_at_overheat = temp;
>
> The & 0xff here is bogus anyway; temp is only ever used as an u8,
> so just declare it as that, or do proper overflow/underflow checking
> on it. The patch will need testing on hardware too, since it changes
> behaviour (it should be a bugfix, but who knows).
Maybe someone can test this?
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..2a2de73 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10) & 0xFF;
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 9:59 ` Roel Kluin
@ 2008-03-10 18:13 ` Darrick J. Wong
-1 siblings, 0 replies; 17+ messages in thread
From: Darrick J. Wong @ 2008-03-10 18:13 UTC (permalink / raw)
To: Roel Kluin; +Cc: Colin Leroy, lkml, linuxppc-dev
On Mon, Mar 10, 2008 at 10:59:43AM +0100, Roel Kluin wrote:
> > The & 0xff here is bogus anyway; temp is only ever used as an u8,
> > so just declare it as that, or do proper overflow/underflow checking
> > on it. The patch will need testing on hardware too, since it changes
> > behaviour (it should be a bugfix, but who knows).
>
> Maybe someone can test this?
I did. No regressions observed and it fixes that bug as well. Sorry I
didn't catch it earlier... :/
Acked-by: Darrick J. Wong <djwong@us.ibm.com>
--D
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 18:13 ` Darrick J. Wong
0 siblings, 0 replies; 17+ messages in thread
From: Darrick J. Wong @ 2008-03-10 18:13 UTC (permalink / raw)
To: Roel Kluin; +Cc: Segher Boessenkool, benh, linuxppc-dev, lkml, Colin Leroy
On Mon, Mar 10, 2008 at 10:59:43AM +0100, Roel Kluin wrote:
> > The & 0xff here is bogus anyway; temp is only ever used as an u8,
> > so just declare it as that, or do proper overflow/underflow checking
> > on it. The patch will need testing on hardware too, since it changes
> > behaviour (it should be a bugfix, but who knows).
>
> Maybe someone can test this?
I did. No regressions observed and it fixes that bug as well. Sorry I
didn't catch it earlier... :/
Acked-by: Darrick J. Wong <djwong@us.ibm.com>
--D
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 9:13 ` Segher Boessenkool
@ 2008-03-10 21:17 ` Roel Kluin
-1 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 21:17 UTC (permalink / raw)
To: Segher Boessenkool, akpm; +Cc: Colin Leroy, Darrick J. Wong, lkml, linuxppc-dev
Andrew, I think you may want this patch instead of the other
adt746x-logical-bitwise-confusion-in-set_max_duty_at_crit.patch
It includes suggested changes by Segher Boessenkool and I think this
version was tested by Darrick J. Wong
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..2a2de73 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10) & 0xFF;
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 21:17 ` Roel Kluin
0 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 21:17 UTC (permalink / raw)
To: Segher Boessenkool, akpm
Cc: benh, linuxppc-dev, lkml, Colin Leroy, Darrick J. Wong
Andrew, I think you may want this patch instead of the other
adt746x-logical-bitwise-confusion-in-set_max_duty_at_crit.patch
It includes suggested changes by Segher Boessenkool and I think this
version was tested by Darrick J. Wong
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..2a2de73 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10) & 0xFF;
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 21:17 ` Roel Kluin
@ 2008-03-10 21:56 ` Segher Boessenkool
-1 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2008-03-10 21:56 UTC (permalink / raw)
To: Roel Kluin; +Cc: lkml, linuxppc-dev, Colin Leroy, akpm, Darrick J. Wong
> It includes suggested changes by Segher Boessenkool and I think this
> version was tested by Darrick J. Wong
> - u8 reg;
> + u8 reg, temp;
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7473_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> - temp = temp && 0xFF;
> +
> + temp = simple_strtol(buf, NULL, 10) & 0xFF;
It still does this superfluous "& 0xff", which hides the lack of
range checking.
Segher
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 21:56 ` Segher Boessenkool
0 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2008-03-10 21:56 UTC (permalink / raw)
To: Roel Kluin; +Cc: akpm, Darrick J. Wong, benh, linuxppc-dev, lkml, Colin Leroy
> It includes suggested changes by Segher Boessenkool and I think this
> version was tested by Darrick J. Wong
> - u8 reg;
> + u8 reg, temp;
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7473_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> - temp = temp && 0xFF;
> +
> + temp = simple_strtol(buf, NULL, 10) & 0xFF;
It still does this superfluous "& 0xff", which hides the lack of
range checking.
Segher
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 21:56 ` Segher Boessenkool
@ 2008-03-10 22:15 ` Roel Kluin
-1 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 22:15 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: lkml, linuxppc-dev, Colin Leroy, akpm, Darrick J. Wong
Segher Boessenkool wrote:
>> It includes suggested changes by Segher Boessenkool and I think this
>> version was tested by Darrick J. Wong
>
>> - u8 reg;
>> + u8 reg, temp;
>> struct i2c_client *client = to_i2c_client(dev);
>> struct adt7473_data *data = i2c_get_clientdata(client);
>> - int temp = simple_strtol(buf, NULL, 10);
>> - temp = temp && 0xFF;
>> +
>> + temp = simple_strtol(buf, NULL, 10) & 0xFF;
>
> It still does this superfluous "& 0xff", which hides the lack of
> range checking.
Sorry didn't quite grep that
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..98937d3 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10);
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 22:15 ` Roel Kluin
0 siblings, 0 replies; 17+ messages in thread
From: Roel Kluin @ 2008-03-10 22:15 UTC (permalink / raw)
To: Segher Boessenkool
Cc: akpm, Darrick J. Wong, benh, linuxppc-dev, lkml, Colin Leroy
Segher Boessenkool wrote:
>> It includes suggested changes by Segher Boessenkool and I think this
>> version was tested by Darrick J. Wong
>
>> - u8 reg;
>> + u8 reg, temp;
>> struct i2c_client *client = to_i2c_client(dev);
>> struct adt7473_data *data = i2c_get_clientdata(client);
>> - int temp = simple_strtol(buf, NULL, 10);
>> - temp = temp && 0xFF;
>> +
>> + temp = simple_strtol(buf, NULL, 10) & 0xFF;
>
> It still does this superfluous "& 0xff", which hides the lack of
> range checking.
Sorry didn't quite grep that
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..98937d3 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -566,11 +566,11 @@ static ssize_t set_max_duty_at_crit(struct device *dev,
const char *buf,
size_t count)
{
- u8 reg;
+ u8 reg, temp;
struct i2c_client *client = to_i2c_client(dev);
struct adt7473_data *data = i2c_get_clientdata(client);
- int temp = simple_strtol(buf, NULL, 10);
- temp = temp && 0xFF;
+
+ temp = simple_strtol(buf, NULL, 10);
mutex_lock(&data->lock);
data->max_duty_at_overheat = temp;
^ permalink raw reply related [flat|nested] 17+ messages in thread