right bit shift error in ad7314.c ?

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Jeroen,

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 -- [email protected]

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Geert,

----- Original Message -----

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html