* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() [not found] ` <1205136413.8621.3.camel@pasglop> @ 2008-03-10 8:22 ` Roel Kluin 2008-03-10 9:13 ` Segher Boessenkool 0 siblings, 1 reply; 7+ messages in thread From: Roel Kluin @ 2008-03-10 8:22 UTC (permalink / raw) To: benh; +Cc: Colin Leroy, linuxppc-dev, lkml 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] 7+ messages in thread
* Re: ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() 2008-03-10 8:22 ` ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() 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; 7+ messages in thread From: Segher Boessenkool @ 2008-03-10 9:13 UTC (permalink / raw) To: Roel Kluin; +Cc: Colin Leroy, lkml, linuxppc-dev > 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2008-03-10 22:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <47D47B11.1000303@tiscali.nl>
[not found] ` <20080310084633.5246ecfe@paperstreet.colino.net>
[not found] ` <1205136413.8621.3.camel@pasglop>
2008-03-10 8:22 ` ADT746X: logical-bitwise & confusion in set_max_duty_at_crit() 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).