logical-bitwise & confusion
Signed-off-by: Roel Kluin <[email protected]>
---
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;
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 [email protected], it should be picked up
anyway.
Ben.
> > Signed-off-by: Roel Kluin <[email protected]>
> > ---
> > 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;
> >
>
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 <[email protected]>
> ---
> 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
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 [email protected], it should be picked up
> anyway.
(linuxppc-dev CC'd)
---
logical-bitwise & confusion
Signed-off-by: Roel Kluin <[email protected]>
---
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;
> 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
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 <[email protected]>
---
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;
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 <[email protected]>
--D
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 <[email protected]>
---
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;
> 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
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 <[email protected]>
---
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;