linux-kernel - Re: [PATCH] iio: temperature: mcp9600: Fix temperature reading for negative values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 27 Apr 2024 21:57:58 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Andrew Hepp <andrew.hepp@...pp.dev>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: temperature: mcp9600: Fix temperature reading for
 negative values

Am Sat, Apr 27, 2024 at 05:49:18AM -0300 schrieb Marcelo Schmitt:
> Hi Dimitri,
> 
> Interesting patch this one.
> I think this does apply, although, the cold junction register has for sign bits
> so I think we could also have a mask to clear those out.
> Some code suggestions inline.
>
Hi Marcelo,

the temperature bits are in two’s complement format for hot and cold
junction. Equations to calculate the temperature are also the same in
the datasheet. There should be no difference when handling them. I don't
think we need to do anything more with the value except sign_extend it to
the appropriate data type. If the sign bits aren't right, there is a bug
in the chip, until then futher processing of it is unneeded. I could add
a comment here if it helps.

> On 04/24, Dimitri Fedrau wrote:
> > Temperature is stored as 16bit value in two's complement format. Current
> > implementation ignores the sign bit. Make it aware of the sign bit by
> > using sign_extend32.
> > 
> > Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter")
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> > ---
> >  drivers/iio/temperature/mcp9600.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index 46845804292b..7a3eef5d5e75 100644
> > --- a/drivers/iio/temperature/mcp9600.c
> > +++ b/drivers/iio/temperature/mcp9600.c
> 
> #define MCP9600_COLD_JUNCTION_SIGN_MSK GENMASK(15,12)
> ...
> 
> > @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data,
> >  
> >  	if (ret < 0)
> >  		return ret;
> > -	*val = ret;
> > +
> > +	*val = sign_extend32(ret, 15);
> 	if (chan->address == MCP9600_COLD_JUNCTION)
> 		*val &= ~MCP9600_COLD_JUNCTION_SIGN_MSK;
>
This won't work. Assuming int is 32-bit ret = 0xfffe and *val = -2 after
sign_extends32, this would result in *val = -61442 which is wrong.
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.39.2
> > 
> > 
Best regards,
Dimitri Fedrau