[1/1] w1: w1 temp calculation overflow fix.

[1/1] w1: w1 temp calculation overflow fix.

[Evgeniy Polyakov]

Signed-off-by: Ian Dall <ian@beware.dropbear.id.au>
Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 2c8dff9..1ed3d55 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -115,7 +115,7 @@ static struct w1_therm_family_converter w1_therm_families[] = {
 
 static inline int w1_DS18B20_convert_temp(u8 rom[9])
 {
-	s16 t = (rom[1] << 8) | rom[0];
+	int t = ((s16)rom[1] << 8) | rom[0];
 	t = t*1000/16;
 	return t;
 }


-- 
	Evgeniy Polyakov

Re: [1/1] w1: w1 temp calculation overflow fix.

[Harvey Harrison]

On Tue, 2009-02-10 at 00:42 +0300, Evgeniy Polyakov wrote:
> Signed-off-by: Ian Dall <ian@beware.dropbear.id.au>
> Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 2c8dff9..1ed3d55 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -115,7 +115,7 @@ static struct w1_therm_family_converter w1_therm_families[] = {
>  
>  static inline int w1_DS18B20_convert_temp(u8 rom[9])
>  {
> -	s16 t = (rom[1] << 8) | rom[0];
> +	int t = ((s16)rom[1] << 8) | rom[0];

Alternatively,

	int t = (s16)le16_to_cpup((__le16 *)rom);

Cheers,

Harvey

Re: [1/1] w1: w1 temp calculation overflow fix.

[Evgeniy Polyakov]

On Mon, Feb 09, 2009 at 01:48:44PM -0800, Harvey Harrison (harvey.harrison@gmail.com) wrote:
> >  {
> > -	s16 t = (rom[1] << 8) | rom[0];
> > +	int t = ((s16)rom[1] << 8) | rom[0];
> 
> Alternatively,
> 
> 	int t = (s16)le16_to_cpup((__le16 *)rom);

Well, yes, but for the consistency with the other conversation functions
I think above is more appropriate.

-- 
	Evgeniy Polyakov

Re: [1/1] w1: w1 temp calculation overflow fix.

[Andrew Morton]

On Tue, 10 Feb 2009 00:42:17 +0300
Evgeniy Polyakov <zbr@ioremap.net> wrote:

> Signed-off-by: Ian Dall <ian@beware.dropbear.id.au>
> Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>

I assumed from the above that Ian authored this patch.  Please let me
know if that was incorrect.

The way to track authorship is to put the originator's From: line at
the top of the changelog.

> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 2c8dff9..1ed3d55 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -115,7 +115,7 @@ static struct w1_therm_family_converter w1_therm_families[] = {
>  
>  static inline int w1_DS18B20_convert_temp(u8 rom[9])
>  {
> -	s16 t = (rom[1] << 8) | rom[0];
> +	int t = ((s16)rom[1] << 8) | rom[0];
>  	t = t*1000/16;
>  	return t;
>  }

It seems strange to use s16 here, but it will fix the bug.

Perhaps this function should be using plain old `unsigned' everywhere.

Please provide changelogs.  This bugfix is applicable to 2.6.28.x and
probably earlier.  But due to the lack of any supporting information I
am not in a position to determine whether it should be backported.

Re: [1/1] w1: w1 temp calculation overflow fix.

[Evgeniy Polyakov]

On Mon, Feb 09, 2009 at 01:56:41PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> > Signed-off-by: Ian Dall <ian@beware.dropbear.id.au>
> > Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
> 
> I assumed from the above that Ian authored this patch.  Please let me
> know if that was incorrect.

> The way to track authorship is to put the originator's From: line at
> the top of the changelog.
 
Yes, Ian is the author.
I will put appropriate From: field next time, I did not know it :)

> > --- a/drivers/w1/slaves/w1_therm.c
> > +++ b/drivers/w1/slaves/w1_therm.c
> > @@ -115,7 +115,7 @@ static struct w1_therm_family_converter w1_therm_families[] = {
> >  
> >  static inline int w1_DS18B20_convert_temp(u8 rom[9])
> >  {
> > -	s16 t = (rom[1] << 8) | rom[0];
> > +	int t = ((s16)rom[1] << 8) | rom[0];
> >  	t = t*1000/16;
> >  	return t;
> >  }
> 
> It seems strange to use s16 here, but it will fix the bug.
> 
> Perhaps this function should be using plain old `unsigned' everywhere.
> 
> Please provide changelogs.  This bugfix is applicable to 2.6.28.x and
> probably earlier.  But due to the lack of any supporting information I
> am not in a position to determine whether it should be backported.
 
That's what was placed into the documentation iirc, code for the more
advanced chip works ok. This code exists for way too long, but
apparently did not grow higher than 32 degrees, so it is applicable for
the earlier trees either.

-- 
	Evgeniy Polyakov

Re: [1/1] w1: w1 temp calculation overflow fix.

[Andrew Morton]

On Tue, 10 Feb 2009 01:08:30 +0300
Evgeniy Polyakov <zbr@ioremap.net> wrote:

> > Please provide changelogs.  This bugfix is applicable to 2.6.28.x and
> > probably earlier.  But due to the lack of any supporting information I
> > am not in a position to determine whether it should be backported.
>  
> That's what was placed into the documentation iirc, code for the more
> advanced chip works ok. This code exists for way too long, but
> apparently did not grow higher than 32 degrees, so it is applicable for
> the earlier trees either.

I think the above means that the patch is appropriate for earlier trees.

But we still don't have a _reason_.  Presumably the bug causes the
driver to malfunction in some manner.

There are two reasons for properly describing the patch:

a) the stable maintainers will want to understand why they should
   backport the patch - does the bug affect anyone and if so, how?

b) people who are using earlier kernel versions and who experience a
   bug will want to know whether this patch will fix their bug.  To do
   that, they will need a description of what the patch fixes!

So please, do describe the user-visible impact of the bug if that is at
all relevant or possible.

Re: [1/1] w1: w1 temp calculation overflow fix.

[Andrew Morton]

On Mon, 9 Feb 2009 14:48:03 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> But we still don't have a _reason_.  Presumably the bug causes the
> driver to malfunction in some manner.

ah-hah.  I discovered http://bugzilla.kernel.org/show_bug.cgi?id=12646

Now all is revealed!

Re: [1/1] w1: w1 temp calculation overflow fix.

[Evgeniy Polyakov]

On Mon, Feb 09, 2009 at 03:31:25PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> On Mon, 9 Feb 2009 14:48:03 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > But we still don't have a _reason_.  Presumably the bug causes the
> > driver to malfunction in some manner.
> 
> ah-hah.  I discovered http://bugzilla.kernel.org/show_bug.cgi?id=12646
> 
> Now all is revealed!

Argh, I did not get that you need this description :)
Yes, reading sysfs temperature file may return wrong (overflown) value.

-- 
	Evgeniy Polyakov

Re: [1/1] w1: w1 temp calculation overflow fix.

[Ian Dall]

On Mon, 2009-02-09 at 15:31 -0800, Andrew Morton wrote:
> On Mon, 9 Feb 2009 14:48:03 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > But we still don't have a _reason_.  Presumably the bug causes the
> > driver to malfunction in some manner.
> 
> ah-hah.  I discovered http://bugzilla.kernel.org/show_bug.cgi?id=12646
> 
> Now all is revealed!

I've been out of the loop a couple of days. Is there anything else
needed from me?

Regards,
Ian


-- 
Ian Dall <ian@beware.dropbear.id.au>