Re: [Outreachy kernel] [PATCH v2] iio: temperature: maxim_thermocouple: Fix sparse endianness warnings

[Alison Schofield <amsfield22@gmail.com>]

On Mon, Sep 26, 2016 at 10:44:04PM +0530, sayli karnik wrote:
> Fix following endianness warnings using sparse:
> warning: cast to restricted __be16
> warning: cast to restricted __be32
> Use __be16 type for incoming read data when storage_bytes equals 2 and __be32
> when storage_bytes equals 4. Since the variable allocated already has the
> correct storage size the patch is classified as cosmetic.
> 
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>

Hi Sayli - 

v2 code looks like it handles both cases. 

I think you've made a bug fix!  

When the storage_bytes == 2, it would work on LE systems but give
invalid data on BE systems.  That unsigned int buf points to a location
where only 2 bytes of data are placed during spi_read. Those 2 bytes go
in the upper bytes. Then when the be16_to_cpu() happens that buf
gets cast to 16bits.  We get the upper bytes on LE systems (good bits)
but the lower bytes on BE systems (bad bits)

Lars - Please check my logic and the code too.  If this is a bug
let us know if Sayli should include a fix tag.

Pending Lars' review, you can update the changelog.

Take a further look at storage_bytes and you'll find when it is
set to 2 and when it is set to 4.  That would be good information
to include in the changelog.  You'll find that this is only a bug
in one of the two chips this driver supports.

Check your precommit checkpatch hooks.  There are still CHECKs in
the spi_read lines of code.

And, back to the less exciting. It would be good for that commit
message to say what you did: replace unsigned int with __be16/32
I know you're short of space on that line.  It's ok to drop the
subdir (temperature) and just state the driver as in:
iio: maxim_thermocouple: commit message.

Please 'CC linux-iio on v3 and the module author, from bottom of
source file.

Thanks Sayli,
alisons

> ---
> v2:
> Updated the subject line and changelog
> Moved the spi_read function call inside respective case statements so that data
> is read only once.
> Moved the return statement after the switch case
> 
>  drivers/iio/temperature/maxim_thermocouple.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/temperature/maxim_thermocouple.c b/drivers/iio/temperature/maxim_thermocouple.c
> index 39dd202..2683b0f 100644
> --- a/drivers/iio/temperature/maxim_thermocouple.c
> +++ b/drivers/iio/temperature/maxim_thermocouple.c
> @@ -123,22 +123,24 @@ static int maxim_thermocouple_read(struct maxim_thermocouple_data *data,
>  {
>  	unsigned int storage_bytes = data->chip->read_size;
>  	unsigned int shift = chan->scan_type.shift + (chan->address * 8);
> -	unsigned int buf;
> +	__be16 buf1;
> +	__be32 buf2;
>  	int ret;
>  
> -	ret = spi_read(data->spi, (void *) &buf, storage_bytes);
> -	if (ret)
> -		return ret;
> -
>  	switch (storage_bytes) {
>  	case 2:
> -		*val = be16_to_cpu(buf);
> +		ret = spi_read(data->spi, (void *) &buf1, storage_bytes);
> +		*val = be16_to_cpu(buf1);
>  		break;
>  	case 4:
> -		*val = be32_to_cpu(buf);
> +		ret = spi_read(data->spi, (void *) &buf2, storage_bytes);
> +		*val = be32_to_cpu(buf2);
>  		break;
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	/* check to be sure this is a valid reading */
>  	if (*val & data->chip->status_bit)
>  		return -EINVAL;
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160926171404.GA11773%40sayli-HP-15-Notebook-PC.
> For more options, visit https://groups.google.com/d/optout.