| Message ID | 20191028200700.213753-5-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Consolidate {get,put}_unaligned_[bl]e24() definitions | expand |
On Mon, 28 Oct 2019 13:06:55 -0700 Bart Van Assche <bvanassche@acm.org> 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 <jic23@kernel.org> > Cc: Hartmut Knaack <knaack.h@gmx.de> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > 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.
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);
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 <jic23@kernel.org> Cc: Hartmut Knaack <knaack.h@gmx.de> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/iio/common/st_sensors/st_sensors_core.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)