LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
@ 2008-03-10 0:04 Roel Kluin
2008-03-10 7:46 ` Colin Leroy
0 siblings, 1 reply; 10+ messages in thread
From: Roel Kluin @ 2008-03-10 0:04 UTC (permalink / raw)
To: colin; +Cc: lkml
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..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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 0:04 ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() Roel Kluin
@ 2008-03-10 7:46 ` Colin Leroy
2008-03-10 8:06 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 10+ messages in thread
From: Colin Leroy @ 2008-03-10 7:46 UTC (permalink / raw)
To: Roel Kluin; +Cc: lkml, Benjamin Herrenschmidt
On Mon, 10 Mar 2008 01:04:33 +0100, Roel Kluin wrote:
Hi,
> logical-bitwise & confusion
Looks good to me, but I'm not really maintaining that anymore :-)
I'm not sure who does, Cc:ing Benjamin as he'll probably know.
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> ---
> 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;
>
--
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 7:46 ` Colin Leroy
@ 2008-03-10 8:06 ` Benjamin Herrenschmidt
2008-03-10 8:22 ` Roel Kluin
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Herrenschmidt @ 2008-03-10 8:06 UTC (permalink / raw)
To: Colin Leroy; +Cc: Roel Kluin, lkml
On Mon, 2008-03-10 at 08:46 +0100, Colin Leroy wrote:
> On Mon, 10 Mar 2008 01:04:33 +0100, Roel Kluin wrote:
>
> Hi,
>
> > logical-bitwise & confusion
>
> Looks good to me, but I'm not really maintaining that anymore :-)
> I'm not sure who does, Cc:ing Benjamin as he'll probably know.
Nobody is U suspect...
Send it to the list linuxppc-dev@ozlabs.org, it should be picked up
anyway.
Ben.
> > Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> > ---
> > 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;
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 8:06 ` Benjamin Herrenschmidt
@ 2008-03-10 8:22 ` Roel Kluin
2008-03-10 9:13 ` Segher Boessenkool
0 siblings, 1 reply; 10+ messages in thread
From: Roel Kluin @ 2008-03-10 8:22 UTC (permalink / raw)
To: benh; +Cc: Colin Leroy, lkml, linuxppc-dev
Benjamin Herrenschmidt wrote:
> On Mon, 2008-03-10 at 08:46 +0100, Colin Leroy wrote:
>> On Mon, 10 Mar 2008 01:04:33 +0100, Roel Kluin wrote:
>>
>>> logical-bitwise & confusion
>> Looks good to me, but I'm not really maintaining that anymore :-)
>> I'm not sure who does, Cc:ing Benjamin as he'll probably know.
>
> Nobody is U suspect...
>
> Send it to the list linuxppc-dev@ozlabs.org, it should be picked up
> anyway.
(linuxppc-dev CC'd)
---
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..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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit()
2008-03-10 8:22 ` Roel Kluin
@ 2008-03-10 9:13 ` Segher Boessenkool
2008-03-10 9:59 ` Roel Kluin
2008-03-10 21:17 ` Roel Kluin
0 siblings, 2 replies; 10+ 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] 10+ 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
2008-03-10 18:13 ` Darrick J. Wong
2008-03-10 21:17 ` Roel Kluin
1 sibling, 1 reply; 10+ 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 [flat|nested] 10+ 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
0 siblings, 0 replies; 10+ 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] 10+ 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
@ 2008-03-10 21:17 ` Roel Kluin
2008-03-10 21:56 ` Segher Boessenkool
1 sibling, 1 reply; 10+ 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 [flat|nested] 10+ 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
2008-03-10 22:15 ` Roel Kluin
0 siblings, 1 reply; 10+ 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] 10+ 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
0 siblings, 0 replies; 10+ 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 [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-03-10 22:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-10 0:04 ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() Roel Kluin
2008-03-10 7:46 ` Colin Leroy
2008-03-10 8:06 ` Benjamin Herrenschmidt
2008-03-10 8:22 ` Roel Kluin
2008-03-10 9:13 ` Segher Boessenkool
2008-03-10 9:59 ` Roel Kluin
2008-03-10 18:13 ` Darrick J. Wong
2008-03-10 21:17 ` Roel Kluin
2008-03-10 21:56 ` Segher Boessenkool
2008-03-10 22:15 ` Roel Kluin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).