right bit shift error in ad7314.c ?

right bit shift error in ad7314.c ?

[Jeroen De Wachter]

Hello,

We've run into some trouble while using the ad7314 driver on an ARM-based
board (big endian).

That driver reads a status from the SPI bus that contains the temperature
read by the sensor. There's some other information in that status too, so
some bit manipulation is done to get the temperature data:

s16 data;
int ret;
...
data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
data = (data << 6) >> 6;

Before that last line is executed, the temperature data is in the lowest
10 bits of the data variable. To be able to handle negative temperatures,
those 10 bits get shifted to the left (discarding higher bits) and then
shifted back to the right, with what is assumed to be an Arithmetic Shift
Right, which takes into account the 2's complement content of the lowest
10 bits.

However, the implementation of right shift on a signed integer is not
defined in the C standard and is implementation-dependent [1].

If I compile the driver from the latest stable kernel (3.17.4) with
gcc 4.8.3, the resulting code does not take the 2's complement content of
the lowest 10 bits into account and uses an *unsigned* bit field extract
instruction:

case ad7314:
        data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
        data = (data << 6) >> 6;
138:   e7e952d5        ubfx    r5, r5, #5, #10

I've tested this with 2 versions of gcc (4.8.3 because it's a recent one,
and 4.4.3 because that's the one we normally use) and they both have this
issue, although 4.4.3 doesn't use the ubfx instruction and does a separate
logical shift left and a *logical* shift right (which also disregards the
2's complement content).

As the standard doesn't specify how to handle a right shift on a signed
integer, the code shouldn't rely on it being an arithmetic shift right.
We've replaced the line

data = (data << 6) >> 6;

with

if (data & 0x0200)
{
        data |= 0xfc00;
}

to avoid the issue. I know this may result in slower code (there's a
branch in there now), but I haven't (yet) figured out a reliable way of
doing this with shift operations only.

I don't know if there's a generic way of performing a right shift that
is guaranteed to be arithmetic?

I can submit a patch if it's considered helpful? There is another spot
in the same driver where a similar operation is performed, I'd fix that
too.

"fun" fact: if I compile the driver without optimization, I get:

case ad7314:
        data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
1fc:   e51b2014        ldr     r2, [fp, #-20]
200:   e3073fe0        movw    r3, #32736      ; 0x7fe0
204:   e0023003        and     r3, r2, r3
208:   e1a032c3        asr     r3, r3, #5
20c:   e14b31b6        strh    r3, [fp, #-22]  ; 0xffffffea
        data = (data << 6) >> 6;
210:   e15b31f6        ldrsh   r3, [fp, #-22]  ; 0xffffffea
214:   e1a03303        lsl     r3, r3, #6
218:   e1a03343        asr     r3, r3, #6
21c:   e14b31b6        strh    r3, [fp, #-22]  ; 0xffffffea

So now it *is* using an Arithmetic Shift Right! This looks like a compiler
bug. The fact that optimized and non-optimized code produce functionally
different results is troublesome. I'll try to send something to the gcc
people soon.

Regards,
Jeroen

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf section 6.5.7
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: right bit shift error in ad7314.c ?

[Geert Uytterhoeven]

Hi Jeroen,

On Tue, Dec 16, 2014 at 3:21 PM, Jeroen De Wachter
<jeroen.de.wachter-CNXmb7IdZIWZIoH1IeqzKA@public.gmane.org> wrote:
> That driver reads a status from the SPI bus that contains the temperature
> read by the sensor. There's some other information in that status too, so
> some bit manipulation is done to get the temperature data:
>
> s16 data;
> int ret;
> ...
> data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
> data = (data << 6) >> 6;
>
> Before that last line is executed, the temperature data is in the lowest
> 10 bits of the data variable. To be able to handle negative temperatures,
> those 10 bits get shifted to the left (discarding higher bits) and then
> shifted back to the right, with what is assumed to be an Arithmetic Shift
> Right, which takes into account the 2's complement content of the lowest
> 10 bits.
>
> However, the implementation of right shift on a signed integer is not
> defined in the C standard and is implementation-dependent [1].

Have you tried using a signed division instead?

The following seems to work for me:

        data = (s16)(data << 6) / 64;

Without the cast, "data << 6" seems to be promoted to an unsigned type.

Alternatively, you can split it in two parts, to force a signed intermediate
result:

        data <<= 6;
        data /= 64;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: right bit shift error in ad7314.c ?

[Jeroen De Wachter]

Hello Geert,

----- Original Message -----
> From: "Geert Uytterhoeven" <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> To: "Jeroen De Wachter" <jeroen.de.wachter-CNXmb7IdZIWZIoH1IeqzKA@public.gmane.org>
> Cc: "linux-spi" <linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "thomas de schampheleire" <thomas.de.schampheleire-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Sent: Tuesday, December 16, 2014 3:53:14 PM
> Subject: Re: right bit shift error in ad7314.c ?
> 
> Hi Jeroen,
> 
> On Tue, Dec 16, 2014 at 3:21 PM, Jeroen De Wachter
> <jeroen.de.wachter-CNXmb7IdZIWZIoH1IeqzKA@public.gmane.org> wrote:
> > That driver reads a status from the SPI bus that contains the temperature
> > read by the sensor. There's some other information in that status too, so
> > some bit manipulation is done to get the temperature data:
> >
> > s16 data;
> > int ret;
> > ...
> > data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
> > data = (data << 6) >> 6;
> >
> > Before that last line is executed, the temperature data is in the lowest
> > 10 bits of the data variable. To be able to handle negative temperatures,
> > those 10 bits get shifted to the left (discarding higher bits) and then
> > shifted back to the right, with what is assumed to be an Arithmetic Shift
> > Right, which takes into account the 2's complement content of the lowest
> > 10 bits.
> >
> > However, the implementation of right shift on a signed integer is not
> > defined in the C standard and is implementation-dependent [1].
> 
> Have you tried using a signed division instead?
> 
> The following seems to work for me:
> 
>         data = (s16)(data << 6) / 64;
> 
> Without the cast, "data << 6" seems to be promoted to an unsigned type.
> 
> Alternatively, you can split it in two parts, to force a signed intermediate
> result:
> 
>         data <<= 6;
>         data /= 64;
> 

Your code should have the same outcome as mine. I had a look at the generated
assembly though and it looks a lot more complicated:

                data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
                data = (s16)(data << 6) / 64;
 154:   e1a03083        lsl     r3, r3, #1

                return sprintf(buf, "%d\n", 250 * data);
 158:   e6bf3073        sxth    r3, r3
 15c:   e283203f        add     r2, r3, #63     ; 0x3f
 160:   e3530000        cmp     r3, #0
 164:   b1a03002        movlt   r3, r2
 168:   e1a02343        asr     r2, r3, #6

vs mine:

                data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
 148:   e1a032a3        lsr     r3, r3, #5
                if (data & 0x0200) {
 14c:   e3130c02        tst     r3, #512        ; 0x200
                  data |= 0xfc00;
 150:   13833b3f        orrne   r3, r3, #64512  ; 0xfc00

I only looked at this on my ARM target (with gcc 4.4.3 and gcc 4.8.3 generating
pretty much the same assembly in both cases), so I don't know what the binary
would look like on other architectures...

There's a conditional instruction there in both solutions (orrne vs. movlt), so
that's not a reason to prefer one over the other.

I do think your version is slightly more readable, although both solutions could
do with a clarifying comment.

So: what do we go for? performance or readability? I don't have a strong
preference, but if I *have* to choose, I'd go with the bitwise manipulation...

Regards,
Jeroen
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html