* 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 related [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 related [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 related [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 related [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 related [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