From the C standard: "The result of E1 >> E2 is E1 right-shifted E2 bit
positions. If E1 has an unsigned type or if E1 has a signed type and a
nonnegative value, the value of the result is the integral part of the
quotient of E1 / 2E2 . If E1 has a signed type and a negative value, the
resulting value is implementation-defined."
Hence use sign_extend_24_to_32() instead of "<< 8 >> 8".
Cc: Jonathan Cameron <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Signed-off-by: Bart Van Assche <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_core.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 4a3064fb6cd9..94a9cec69cd7 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -21,11 +21,6 @@
#include "st_sensors_core.h"
-static inline u32 st_sensors_get_unaligned_le24(const u8 *p)
-{
- return (s32)((p[0] | p[1] << 8 | p[2] << 16) << 8) >> 8;
-}
-
int st_sensors_write_data_with_mask(struct iio_dev *indio_dev,
u8 reg_addr, u8 mask, u8 data)
{
@@ -556,7 +551,7 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
else if (byte_for_channel == 2)
*data = (s16)get_unaligned_le16(outdata);
else if (byte_for_channel == 3)
- *data = (s32)st_sensors_get_unaligned_le24(outdata);
+ *data = get_unaligned_signed_le24(outdata);
st_sensors_free_memory:
kfree(outdata);
--
2.24.0.rc0.303.g954a862665-goog
On Mon, 28 Oct 2019 13:06:55 -0700
Bart Van Assche <[email protected]> wrote:
> From the C standard: "The result of E1 >> E2 is E1 right-shifted E2 bit
> positions. If E1 has an unsigned type or if E1 has a signed type and a
> nonnegative value, the value of the result is the integral part of the
> quotient of E1 / 2E2 . If E1 has a signed type and a negative value, the
> resulting value is implementation-defined."
>
> Hence use sign_extend_24_to_32() instead of "<< 8 >> 8".
+CC linux-iio
>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
> ---
> drivers/iio/common/st_sensors/st_sensors_core.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 4a3064fb6cd9..94a9cec69cd7 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -21,11 +21,6 @@
>
> #include "st_sensors_core.h"
>
> -static inline u32 st_sensors_get_unaligned_le24(const u8 *p)
> -{
> - return (s32)((p[0] | p[1] << 8 | p[2] << 16) << 8) >> 8;
> -}
> -
> int st_sensors_write_data_with_mask(struct iio_dev *indio_dev,
> u8 reg_addr, u8 mask, u8 data)
> {
> @@ -556,7 +551,7 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
> else if (byte_for_channel == 2)
> *data = (s16)get_unaligned_le16(outdata);
> else if (byte_for_channel == 3)
> - *data = (s32)st_sensors_get_unaligned_le24(outdata);
> + *data = get_unaligned_signed_le24(outdata);
>
> st_sensors_free_memory:
> kfree(outdata);
On Mon, Oct 28, 2019 at 01:06:55PM -0700, Bart Van Assche wrote:
> From the C standard: "The result of E1 >> E2 is E1 right-shifted E2 bit
> positions. If E1 has an unsigned type or if E1 has a signed type and a
> nonnegative value, the value of the result is the integral part of the
> quotient of E1 / 2E2 . If E1 has a signed type and a negative value, the
> resulting value is implementation-defined."
FWIW, we actually hard rely on this implementation defined behaviour all
over the kernel. See for example the generic sign_extend{32,64}()
functions.
AFAIR the only reason the C standard says this is implementation defined
is because it wants to support daft things like 1s complement and
saturating integers.
Luckily, Linux doesn't run on any such hardware and we hard rely on
signed being 2s complement and tell the compiler that by using
-fno-strict-overflow (which implies -fwrapv).
And the only sane choice for 2s complement signed shift right is
arithmetic shift right.
(this recently came up in another thread, which I can't remember enough
of to find just now, and I'm not sure we got a GCC person to confirm if
-fwrapv does indeed guarantee arithmetic shift, the GCC documentation
does not mention this)
On 2019-10-30 4:02 p.m., Peter Zijlstra wrote:
> On Mon, Oct 28, 2019 at 01:06:55PM -0700, Bart Van Assche wrote:
>> From the C standard: "The result of E1 >> E2 is E1 right-shifted E2 bit
>> positions. If E1 has an unsigned type or if E1 has a signed type and a
>> nonnegative value, the value of the result is the integral part of the
>> quotient of E1 / 2E2 . If E1 has a signed type and a negative value, the
>> resulting value is implementation-defined."
>
> FWIW, we actually hard rely on this implementation defined behaviour all
> over the kernel. See for example the generic sign_extend{32,64}()
> functions.
>
> AFAIR the only reason the C standard says this is implementation defined
> is because it wants to support daft things like 1s complement and
> saturating integers.
See:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2218.htm
That is in C++20 and on the agenda for C2x:
https://gustedt.wordpress.com/2018/11/12/c2x/
Doug Gilbert
> Luckily, Linux doesn't run on any such hardware and we hard rely on
> signed being 2s complement and tell the compiler that by using
> -fno-strict-overflow (which implies -fwrapv).
>
> And the only sane choice for 2s complement signed shift right is
> arithmetic shift right.
>
> (this recently came up in another thread, which I can't remember enough
> of to find just now, and I'm not sure we got a GCC person to confirm if
> -fwrapv does indeed guarantee arithmetic shift, the GCC documentation
> does not mention this)
>
On 10/30/19 3:13 PM, Douglas Gilbert wrote:
> On 2019-10-30 4:02 p.m., Peter Zijlstra wrote:
>> On Mon, Oct 28, 2019 at 01:06:55PM -0700, Bart Van Assche wrote:
>>> From the C standard: "The result of E1 >> E2 is E1 right-shifted E2 bit
>>> positions. If E1 has an unsigned type or if E1 has a signed type and a
>>> nonnegative value, the value of the result is the integral part of the
>>> quotient of E1 / 2E2 . If E1 has a signed type and a negative value, the
>>> resulting value is implementation-defined."
>>
>> FWIW, we actually hard rely on this implementation defined behaviour all
>> over the kernel. See for example the generic sign_extend{32,64}()
>> functions.
>>
>> AFAIR the only reason the C standard says this is implementation defined
>> is because it wants to support daft things like 1s complement and
>> saturating integers.
>
> See:
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2218.htm
>
> That is in C++20 and on the agenda for C2x:
> https://gustedt.wordpress.com/2018/11/12/c2x/
Thanks Peter and Doug. This is very useful feedback. I will drop the
sign_extend_24_to_32() function and use sign_extend32() instead.
Bart.