* Re: [PATCH v3] iio: temperature: maxim_thermocouple: Replace unsigned int with __be16/__be32 to fix endianness warnings
2016-09-27 20:03 ` [PATCH v3] iio: temperature: maxim_thermocouple: Replace unsigned int with __be16/__be32 to fix endianness warnings Matt Ranostay
@ 2016-09-27 20:21 ` Alison Schofield
2016-09-28 7:19 ` Lars-Peter Clausen
1 sibling, 0 replies; 3+ messages in thread
From: Alison Schofield @ 2016-09-27 20:21 UTC (permalink / raw)
To: Matt Ranostay
Cc: sayli karnik, outreachy-kernel, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio@vger.kernel.org
On Tue, Sep 27, 2016 at 01:03:00PM -0700, Matt Ranostay wrote:
> On Tue, Sep 27, 2016 at 11:10 AM, sayli karnik
> <karniksayli1995@gmail.com> 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.
> > Also, the patch fixes the following bug:
> > Variable 'buf' which is an unsigned int (32 bits) points to a location where
> > only 2 bytes of data are placed during spi_read(). These 2 bytes go in the
> > upper bytes. be16_to_cpu() casts buf to 16 bits. When storage_bytes equals 2,
> > (for thermocouple chip-MAX6675) this will work on Little Endian systems but
> > will give invalid data on Big Endian systems. The patch fixes this bug existing
> > on Big Endian systems for chip MAX6675.
> > Additionally, the patch removes space after a type cast as suggested by
> > checkpatch.pl.
>
> See comments inline.
>
> >
> > Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> > Fixes: 1f25ca11d84a ("iio: temperature: add support for Maxim thermocouple chips")
>
>
> Please CC the linux-iio mailing list in the future :)
>
> > ---
> > Changes in v3:
> > Updated the commit message
> > Updated the changelog to include explanation to the bug fix
> > Fixed the checkpatch issue: CHECK: No space is necessary after a cast
> > Added the fixes tag
> >
> > 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..01cad6e 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);
>
> Meh don't really like having buf1, and buf2 esp since they only used
> in one branch of the switch statement. And waste 2-4 byte of stack :).
>
> I recommend something like:
>
> case 2: {
> __be16 buf;
> ret = spi_read(data->spi, (void *)&buf1, storage_bytes);
> *val = be16_to_cpu(buf1);
> break;
> }
> case 4:
> ....
>
>
> Or one could also change "unsigned int buf" to "__be32 buf"
> And just do the following to fix the big endian system issue:
>
> *val = be32_to_cpu(buf) >> 16;
>
Thanks Matt! (good reason for right cc lists)
Sayli -
What do you think of Matt's second option? It types
the data as __be32 as it comes into the system (although the wrong
size) so that's better endian behavior. The be32_to_cpu() is now done
on the correct type, so sparse should be happy. I'll suggest a short
comment alongside that shift to say you are shifting to align a __be16
alisons
> > 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
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] iio: temperature: maxim_thermocouple: Replace unsigned int with __be16/__be32 to fix endianness warnings
2016-09-27 20:03 ` [PATCH v3] iio: temperature: maxim_thermocouple: Replace unsigned int with __be16/__be32 to fix endianness warnings Matt Ranostay
2016-09-27 20:21 ` Alison Schofield
@ 2016-09-28 7:19 ` Lars-Peter Clausen
1 sibling, 0 replies; 3+ messages in thread
From: Lars-Peter Clausen @ 2016-09-28 7:19 UTC (permalink / raw)
To: Matt Ranostay, sayli karnik
Cc: outreachy-kernel, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, linux-iio@vger.kernel.org
[...]
>>
>> diff --git a/drivers/iio/temperature/maxim_thermocouple.c b/drivers/iio/temperature/maxim_thermocouple.c
>> index 39dd202..01cad6e 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);
>
> Meh don't really like having buf1, and buf2 esp since they only used
> in one branch of the switch statement. And waste 2-4 byte of stack :).
>
> I recommend something like:
>
> case 2: {
> __be16 buf;
> ret = spi_read(data->spi, (void *)&buf1, storage_bytes);
> *val = be16_to_cpu(buf1);
> break;
> }
> case 4:
> ....
>
I'd be quite surprised if the compiled end result actually looks different
between this and the original patch. Compilers are quite smart these days. I
wouldn't worry too much about it and keep the original solution, since it
has a nicer code flow to it.
^ permalink raw reply [flat|nested] 3+ messages in thread