[PATCH 0/9] Consolidate {get,put}_unaligned_[bl]e24() definitions

[PATCH 0/9] Consolidate {get,put}_unaligned_[bl]e24() definitions

[Bart Van Assche]

Hi Peter,

This patch series moves the existing {get,put}_unaligned_[bl]e24() definitions
into include/linux/unaligned/generic.h. This patch series also introduces a function
for sign-extending 24-bit into 32-bit integers and introduces users for all new
functions and macros. Please consider this patch series for kernel version v5.5.

Thanks,

Bart.

Bart Van Assche (9):
  linux/unaligned/byteshift.h: Remove superfluous casts
  c6x: Include <linux/unaligned/generic.h> instead of duplicating it
  treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions
  drivers/iio: Sign extend without triggering implementation-defined
    behavior
  scsi/st: Use get_unaligned_signed_be24()
  scsi/trace: Use get_unaligned_be*()
  arm/ecard: Use get_unaligned_le{16,24}()
  IB/qib: Sign extend without triggering implementation-defined behavior
  ASoC/fsl_spdif: Use put_unaligned_be24() instead of open-coding it

 arch/arm/mach-rpc/ecard.c                     |  18 +--
 arch/c6x/include/asm/unaligned.h              |  65 +--------
 .../iio/common/st_sensors/st_sensors_core.c   |   7 +-
 drivers/infiniband/hw/qib/qib_rc.c            |   2 +-
 drivers/nvme/host/rdma.c                      |   8 --
 drivers/nvme/target/rdma.c                    |   6 -
 drivers/scsi/scsi_trace.c                     | 128 ++++++------------
 drivers/scsi/st.c                             |   4 +-
 drivers/usb/gadget/function/f_mass_storage.c  |   1 +
 drivers/usb/gadget/function/storage_common.h  |   5 -
 include/linux/unaligned/be_byteshift.h        |   6 +-
 include/linux/unaligned/generic.h             |  44 ++++++
 include/linux/unaligned/le_byteshift.h        |   6 +-
 include/target/target_core_backend.h          |   6 -
 sound/soc/fsl/fsl_spdif.c                     |   5 +-
 15 files changed, 103 insertions(+), 208 deletions(-)

[PATCH 1/9] linux/unaligned/byteshift.h: Remove superfluous casts

[Bart Van Assche]

The C language supports casting a void pointer into a non-void pointer
implicitly. Remove explicit void pointer to non-void pointer casts because
these are superfluous.

Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/unaligned/be_byteshift.h | 6 +++---
 include/linux/unaligned/le_byteshift.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/unaligned/be_byteshift.h b/include/linux/unaligned/be_byteshift.h
index 8bdb8fa01bd4..c43ff5918c8a 100644
--- a/include/linux/unaligned/be_byteshift.h
+++ b/include/linux/unaligned/be_byteshift.h
@@ -40,17 +40,17 @@ static inline void __put_unaligned_be64(u64 val, u8 *p)
 
 static inline u16 get_unaligned_be16(const void *p)
 {
-	return __get_unaligned_be16((const u8 *)p);
+	return __get_unaligned_be16(p);
 }
 
 static inline u32 get_unaligned_be32(const void *p)
 {
-	return __get_unaligned_be32((const u8 *)p);
+	return __get_unaligned_be32(p);
 }
 
 static inline u64 get_unaligned_be64(const void *p)
 {
-	return __get_unaligned_be64((const u8 *)p);
+	return __get_unaligned_be64(p);
 }
 
 static inline void put_unaligned_be16(u16 val, void *p)
diff --git a/include/linux/unaligned/le_byteshift.h b/include/linux/unaligned/le_byteshift.h
index 1628b75866f0..2248dcb0df76 100644
--- a/include/linux/unaligned/le_byteshift.h
+++ b/include/linux/unaligned/le_byteshift.h
@@ -40,17 +40,17 @@ static inline void __put_unaligned_le64(u64 val, u8 *p)
 
 static inline u16 get_unaligned_le16(const void *p)
 {
-	return __get_unaligned_le16((const u8 *)p);
+	return __get_unaligned_le16(p);
 }
 
 static inline u32 get_unaligned_le32(const void *p)
 {
-	return __get_unaligned_le32((const u8 *)p);
+	return __get_unaligned_le32(p);
 }
 
 static inline u64 get_unaligned_le64(const void *p)
 {
-	return __get_unaligned_le64((const u8 *)p);
+	return __get_unaligned_le64(p);
 }
 
 static inline void put_unaligned_le16(u16 val, void *p)
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 2/9] c6x: Include <linux/unaligned/generic.h> instead of duplicating it

[Bart Van Assche]

Use the generic __{get,put}_unaligned_[bl]e() definitions instead of
duplicating these. Since a later patch will add more definitions into
<linux/unaligned/generic.h>, this patch ensures that these definitions
have to be added only once. See also commit a7f626c1948a ("C6X: headers").
See also commit 6510d41954dc ("kernel: Move arches to use common unaligned
access").

Cc: Mark Salter <msalter@redhat.com>
Cc: Aurelien Jacquiot <jacquiot.aurelien@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/c6x/include/asm/unaligned.h | 65 +-------------------------------
 1 file changed, 1 insertion(+), 64 deletions(-)

diff --git a/arch/c6x/include/asm/unaligned.h b/arch/c6x/include/asm/unaligned.h
index b56ba7110f5a..d628cc170564 100644
--- a/arch/c6x/include/asm/unaligned.h
+++ b/arch/c6x/include/asm/unaligned.h
@@ -10,6 +10,7 @@
 #define _ASM_C6X_UNALIGNED_H
 
 #include <linux/swab.h>
+#include <linux/unaligned/generic.h>
 
 /*
  * The C64x+ can do unaligned word and dword accesses in hardware
@@ -100,68 +101,4 @@ static inline void put_unaligned64(u64 val, const void *p)
 
 #endif
 
-/*
- * Cause a link-time error if we try an unaligned access other than
- * 1,2,4 or 8 bytes long
- */
-extern int __bad_unaligned_access_size(void);
-
-#define __get_unaligned_le(ptr) (typeof(*(ptr)))({			\
-	sizeof(*(ptr)) == 1 ? *(ptr) :					\
-	  (sizeof(*(ptr)) == 2 ? get_unaligned_le16((ptr)) :		\
-	     (sizeof(*(ptr)) == 4 ? get_unaligned_le32((ptr)) :		\
-		(sizeof(*(ptr)) == 8 ? get_unaligned_le64((ptr)) :	\
-		   __bad_unaligned_access_size())));			\
-	})
-
-#define __get_unaligned_be(ptr) (__force typeof(*(ptr)))({	\
-	sizeof(*(ptr)) == 1 ? *(ptr) :					\
-	  (sizeof(*(ptr)) == 2 ? get_unaligned_be16((ptr)) :		\
-	     (sizeof(*(ptr)) == 4 ? get_unaligned_be32((ptr)) :		\
-		(sizeof(*(ptr)) == 8 ? get_unaligned_be64((ptr)) :	\
-		   __bad_unaligned_access_size())));			\
-	})
-
-#define __put_unaligned_le(val, ptr) ({					\
-	void *__gu_p = (ptr);						\
-	switch (sizeof(*(ptr))) {					\
-	case 1:								\
-		*(u8 *)__gu_p = (__force u8)(val);			\
-		break;							\
-	case 2:								\
-		put_unaligned_le16((__force u16)(val), __gu_p);		\
-		break;							\
-	case 4:								\
-		put_unaligned_le32((__force u32)(val), __gu_p);		\
-		break;							\
-	case 8:								\
-		put_unaligned_le64((__force u64)(val), __gu_p);		\
-		break;							\
-	default:							\
-		__bad_unaligned_access_size();				\
-		break;							\
-	}								\
-	(void)0; })
-
-#define __put_unaligned_be(val, ptr) ({					\
-	void *__gu_p = (ptr);						\
-	switch (sizeof(*(ptr))) {					\
-	case 1:								\
-		*(u8 *)__gu_p = (__force u8)(val);			\
-		break;							\
-	case 2:								\
-		put_unaligned_be16((__force u16)(val), __gu_p);		\
-		break;							\
-	case 4:								\
-		put_unaligned_be32((__force u32)(val), __gu_p);		\
-		break;							\
-	case 8:								\
-		put_unaligned_be64((__force u64)(val), __gu_p);		\
-		break;							\
-	default:							\
-		__bad_unaligned_access_size();				\
-		break;							\
-	}								\
-	(void)0; })
-
 #endif /* _ASM_C6X_UNALIGNED_H */
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 3/9] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions

[Bart Van Assche]

Move the get_unaligned_be24(), get_unaligned_le24() and
put_unaligned_le24() definitions from various drivers into
include/linux/unaligned/generic.h. Add a put_unaligned_be24() and
get_unaligned_signed_[bl]e24() definitions. Change the functions that
depend on get_unaligned_be32() into macros because
<linux/unaligned/generic.h> may be included before get_unaligned_be32()
has been redefined as a macro.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Jens Axboe <axboe@fb.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/nvme/host/rdma.c                     |  8 ----
 drivers/nvme/target/rdma.c                   |  6 ---
 drivers/usb/gadget/function/f_mass_storage.c |  1 +
 drivers/usb/gadget/function/storage_common.h |  5 ---
 include/linux/unaligned/generic.h            | 44 ++++++++++++++++++++
 include/target/target_core_backend.h         |  6 ---
 6 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index dfa07bb9dfeb..66d9c8cc0c5c 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -142,14 +142,6 @@ static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static const struct blk_mq_ops nvme_rdma_mq_ops;
 static const struct blk_mq_ops nvme_rdma_admin_mq_ops;
 
-/* XXX: really should move to a generic header sooner or later.. */
-static inline void put_unaligned_le24(u32 val, u8 *p)
-{
-	*p++ = val;
-	*p++ = val >> 8;
-	*p++ = val >> 16;
-}
-
 static inline int nvme_rdma_queue_idx(struct nvme_rdma_queue *queue)
 {
 	return queue - queue->ctrl->queues;
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 36d906a7f70d..dc193526d4da 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -143,12 +143,6 @@ static int num_pages(int len)
 	return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
 }
 
-/* XXX: really should move to a generic header sooner or later.. */
-static inline u32 get_unaligned_le24(const u8 *p)
-{
-	return (u32)p[0] | (u32)p[1] << 8 | (u32)p[2] << 16;
-}
-
 static inline bool nvmet_rdma_need_data_in(struct nvmet_rdma_rsp *rsp)
 {
 	return nvme_is_write(rsp->req.cmd) &&
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 7c96c4665178..950d2a85f098 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -216,6 +216,7 @@
 #include <linux/freezer.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <asm/unaligned.h>
 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h
index e5e3a2553aaa..bdeb1e233fc9 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -172,11 +172,6 @@ enum data_direction {
 	DATA_DIR_NONE
 };
 
-static inline u32 get_unaligned_be24(u8 *buf)
-{
-	return 0xffffff & (u32) get_unaligned_be32(buf - 1);
-}
-
 static inline struct fsg_lun *fsg_lun_from_dev(struct device *dev)
 {
 	return container_of(dev, struct fsg_lun, dev);
diff --git a/include/linux/unaligned/generic.h b/include/linux/unaligned/generic.h
index 57d3114656e5..f7fa3f248c85 100644
--- a/include/linux/unaligned/generic.h
+++ b/include/linux/unaligned/generic.h
@@ -2,6 +2,8 @@
 #ifndef _LINUX_UNALIGNED_GENERIC_H
 #define _LINUX_UNALIGNED_GENERIC_H
 
+#include <linux/types.h>
+
 /*
  * Cause a link-time error if we try an unaligned access other than
  * 1,2,4 or 8 bytes long
@@ -66,4 +68,46 @@ extern void __bad_unaligned_access_size(void);
 	}								\
 	(void)0; })
 
+/* Only use get_unaligned_be24() if reading p - 1 is allowed. */
+#define get_unaligned_be24(p) (get_unaligned_be32((p) - 1) & 0xffffffu)
+
+#define get_unaligned_le24(p) (get_unaligned_le32((p)) & 0xffffffu)
+
+/* Sign-extend a 24-bit into a 32-bit integer. */
+static inline s32 sign_extend_24_to_32(u32 i)
+{
+	i &= 0xffffffu;
+	return i - ((i >> 23) << 24);
+}
+
+#define get_unaligned_signed_be24(p)			\
+	sign_extend_24_to_32(get_unaligned_be24((p)))
+
+#define get_unaligned_signed_le24(p)			\
+	sign_extend_24_to_32(get_unaligned_le24((p)))
+
+static inline void __put_unaligned_be24(u32 val, u8 *p)
+{
+	*p++ = val >> 16;
+	*p++ = val >> 8;
+	*p++ = val;
+}
+
+static inline void put_unaligned_be24(u32 val, void *p)
+{
+	__put_unaligned_be24(val, p);
+}
+
+static inline void __put_unaligned_le24(u32 val, u8 *p)
+{
+	*p++ = val;
+	*p++ = val >> 8;
+	*p++ = val >> 16;
+}
+
+static inline void put_unaligned_le24(u32 val, void *p)
+{
+	__put_unaligned_le24(val, p);
+}
+
 #endif /* _LINUX_UNALIGNED_GENERIC_H */
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 51b6f50eabee..1b752d8ea529 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -116,10 +116,4 @@ static inline bool target_dev_configured(struct se_device *se_dev)
 	return !!(se_dev->dev_flags & DF_CONFIGURED);
 }
 
-/* Only use get_unaligned_be24() if reading p - 1 is allowed. */
-static inline uint32_t get_unaligned_be24(const uint8_t *const p)
-{
-	return get_unaligned_be32(p - 1) & 0xffffffU;
-}
-
 #endif /* TARGET_CORE_BACKEND_H */
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 4/9] drivers/iio: Sign extend without triggering implementation-defined behavior

[Bart Van Assche]

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(-)

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

[PATCH 5/9] scsi/st: Use get_unaligned_signed_be24()

[Bart Van Assche]

Use this function instead of open-coding it.

Cc: Kai Makisara <Kai.Makisara@kolumbus.fi>
Cc: James E.J. Bottomley <jejb@linux.ibm.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/st.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e3266a64a477..53dc7706c935 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -44,6 +44,7 @@ static const char *verstr = "20160209";
 
 #include <linux/uaccess.h>
 #include <asm/dma.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
@@ -2679,8 +2680,7 @@ static void deb_space_print(struct scsi_tape *STp, int direction, char *units, u
 	if (!debugging)
 		return;
 
-	sc = cmd[2] & 0x80 ? 0xff000000 : 0;
-	sc |= (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
+	sc = get_unaligned_signed_be24(&cmd[2]);
 	if (direction)
 		sc = -sc;
 	st_printk(ST_DEB_MSG, STp, "Spacing tape %s over %d %s.\n",
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 6/9] scsi/trace: Use get_unaligned_be*()

[Bart Van Assche]

This patch fixes an unintended sign extension on left shifts. From Colin
King: "Shifting a u8 left will cause the value to be promoted to an
integer. If the top bit of the u8 is set then the following conversion to
an u64 will sign extend the value causing the upper 32 bits to be set in
the result."

Fix this by using get_unaligned_be*() instead.

Additionally, fix handling of TRANSFER LENGTH == 0 for READ(6) and
WRITE(6).

Cc: Christoph Hellwig <hch@lst.de>
Cc: James E.J. Bottomley <jejb@linux.ibm.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: bf8162354233 ("[SCSI] add scsi trace core functions and put trace points")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_trace.c | 128 ++++++++++++--------------------------
 1 file changed, 41 insertions(+), 87 deletions(-)

diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c
index 0f17e7dac1b0..24c9c504e42c 100644
--- a/drivers/scsi/scsi_trace.c
+++ b/drivers/scsi/scsi_trace.c
@@ -9,7 +9,7 @@
 #include <trace/events/scsi.h>
 
 #define SERVICE_ACTION16(cdb) (cdb[1] & 0x1f)
-#define SERVICE_ACTION32(cdb) ((cdb[8] << 8) | cdb[9])
+#define SERVICE_ACTION32(cdb) (get_unaligned_be16(&cdb[8]))
 
 static const char *
 scsi_trace_misc(struct trace_seq *, unsigned char *, int);
@@ -18,15 +18,16 @@ static const char *
 scsi_trace_rw6(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	sector_t lba = 0, txlen = 0;
+	u32 lba, txlen;
 
-	lba |= ((cdb[1] & 0x1F) << 16);
-	lba |=  (cdb[2] << 8);
-	lba |=   cdb[3];
-	txlen = cdb[4];
+	lba = get_unaligned_be24(&cdb[1]) & 0x1fffff;
+	/*
+	 * From SBC-2: a TRANSFER LENGTH field set to zero specifies that 256
+	 * logical blocks shall be read (READ(6)) or written (WRITE(6)).
+	 */
+	txlen = cdb[4] ? : 256;
 
-	trace_seq_printf(p, "lba=%llu txlen=%llu",
-			 (unsigned long long)lba, (unsigned long long)txlen);
+	trace_seq_printf(p, "lba=%u txlen=%u", lba, txlen);
 	trace_seq_putc(p, 0);
 
 	return ret;
@@ -36,17 +37,12 @@ static const char *
 scsi_trace_rw10(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	sector_t lba = 0, txlen = 0;
+	u32 lba, txlen;
 
-	lba |= (cdb[2] << 24);
-	lba |= (cdb[3] << 16);
-	lba |= (cdb[4] << 8);
-	lba |=  cdb[5];
-	txlen |= (cdb[7] << 8);
-	txlen |=  cdb[8];
+	lba = get_unaligned_be32(&cdb[2]);
+	txlen = get_unaligned_be16(&cdb[7]);
 
-	trace_seq_printf(p, "lba=%llu txlen=%llu protect=%u",
-			 (unsigned long long)lba, (unsigned long long)txlen,
+	trace_seq_printf(p, "lba=%u txlen=%u protect=%u", lba, txlen,
 			 cdb[1] >> 5);
 
 	if (cdb[0] == WRITE_SAME)
@@ -61,19 +57,12 @@ static const char *
 scsi_trace_rw12(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	sector_t lba = 0, txlen = 0;
-
-	lba |= (cdb[2] << 24);
-	lba |= (cdb[3] << 16);
-	lba |= (cdb[4] << 8);
-	lba |=  cdb[5];
-	txlen |= (cdb[6] << 24);
-	txlen |= (cdb[7] << 16);
-	txlen |= (cdb[8] << 8);
-	txlen |=  cdb[9];
-
-	trace_seq_printf(p, "lba=%llu txlen=%llu protect=%u",
-			 (unsigned long long)lba, (unsigned long long)txlen,
+	u32 lba, txlen;
+
+	lba = get_unaligned_be32(&cdb[2]);
+	txlen = get_unaligned_be32(&cdb[6]);
+
+	trace_seq_printf(p, "lba=%u txlen=%u protect=%u", lba, txlen,
 			 cdb[1] >> 5);
 	trace_seq_putc(p, 0);
 
@@ -84,23 +73,13 @@ static const char *
 scsi_trace_rw16(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	sector_t lba = 0, txlen = 0;
-
-	lba |= ((u64)cdb[2] << 56);
-	lba |= ((u64)cdb[3] << 48);
-	lba |= ((u64)cdb[4] << 40);
-	lba |= ((u64)cdb[5] << 32);
-	lba |= (cdb[6] << 24);
-	lba |= (cdb[7] << 16);
-	lba |= (cdb[8] << 8);
-	lba |=  cdb[9];
-	txlen |= (cdb[10] << 24);
-	txlen |= (cdb[11] << 16);
-	txlen |= (cdb[12] << 8);
-	txlen |=  cdb[13];
-
-	trace_seq_printf(p, "lba=%llu txlen=%llu protect=%u",
-			 (unsigned long long)lba, (unsigned long long)txlen,
+	u64 lba;
+	u32 txlen;
+
+	lba = get_unaligned_be64(&cdb[2]);
+	txlen = get_unaligned_be32(&cdb[10]);
+
+	trace_seq_printf(p, "lba=%llu txlen=%u protect=%u", lba, txlen,
 			 cdb[1] >> 5);
 
 	if (cdb[0] == WRITE_SAME_16)
@@ -115,8 +94,8 @@ static const char *
 scsi_trace_rw32(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p), *cmd;
-	sector_t lba = 0, txlen = 0;
-	u32 ei_lbrt = 0;
+	u64 lba;
+	u32 ei_lbrt, txlen;
 
 	switch (SERVICE_ACTION32(cdb)) {
 	case READ_32:
@@ -136,26 +115,12 @@ scsi_trace_rw32(struct trace_seq *p, unsigned char *cdb, int len)
 		goto out;
 	}
 
-	lba |= ((u64)cdb[12] << 56);
-	lba |= ((u64)cdb[13] << 48);
-	lba |= ((u64)cdb[14] << 40);
-	lba |= ((u64)cdb[15] << 32);
-	lba |= (cdb[16] << 24);
-	lba |= (cdb[17] << 16);
-	lba |= (cdb[18] << 8);
-	lba |=  cdb[19];
-	ei_lbrt |= (cdb[20] << 24);
-	ei_lbrt |= (cdb[21] << 16);
-	ei_lbrt |= (cdb[22] << 8);
-	ei_lbrt |=  cdb[23];
-	txlen |= (cdb[28] << 24);
-	txlen |= (cdb[29] << 16);
-	txlen |= (cdb[30] << 8);
-	txlen |=  cdb[31];
-
-	trace_seq_printf(p, "%s_32 lba=%llu txlen=%llu protect=%u ei_lbrt=%u",
-			 cmd, (unsigned long long)lba,
-			 (unsigned long long)txlen, cdb[10] >> 5, ei_lbrt);
+	lba = get_unaligned_be64(&cdb[12]);
+	ei_lbrt = get_unaligned_be32(&cdb[20]);
+	txlen = get_unaligned_be32(&cdb[28]);
+
+	trace_seq_printf(p, "%s_32 lba=%llu txlen=%u protect=%u ei_lbrt=%u",
+			 cmd, lba, txlen, cdb[10] >> 5, ei_lbrt);
 
 	if (SERVICE_ACTION32(cdb) == WRITE_SAME_32)
 		trace_seq_printf(p, " unmap=%u", cdb[10] >> 3 & 1);
@@ -170,7 +135,7 @@ static const char *
 scsi_trace_unmap(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	unsigned int regions = cdb[7] << 8 | cdb[8];
+	unsigned int regions = get_unaligned_be16(&cdb[7]);
 
 	trace_seq_printf(p, "regions=%u", (regions - 8) / 16);
 	trace_seq_putc(p, 0);
@@ -182,8 +147,8 @@ static const char *
 scsi_trace_service_action_in(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p), *cmd;
-	sector_t lba = 0;
-	u32 alloc_len = 0;
+	u64 lba;
+	u32 alloc_len;
 
 	switch (SERVICE_ACTION16(cdb)) {
 	case SAI_READ_CAPACITY_16:
@@ -197,21 +162,10 @@ scsi_trace_service_action_in(struct trace_seq *p, unsigned char *cdb, int len)
 		goto out;
 	}
 
-	lba |= ((u64)cdb[2] << 56);
-	lba |= ((u64)cdb[3] << 48);
-	lba |= ((u64)cdb[4] << 40);
-	lba |= ((u64)cdb[5] << 32);
-	lba |= (cdb[6] << 24);
-	lba |= (cdb[7] << 16);
-	lba |= (cdb[8] << 8);
-	lba |=  cdb[9];
-	alloc_len |= (cdb[10] << 24);
-	alloc_len |= (cdb[11] << 16);
-	alloc_len |= (cdb[12] << 8);
-	alloc_len |=  cdb[13];
-
-	trace_seq_printf(p, "%s lba=%llu alloc_len=%u", cmd,
-			 (unsigned long long)lba, alloc_len);
+	lba = get_unaligned_be64(&cdb[2]);
+	alloc_len = get_unaligned_be32(&cdb[10]);
+
+	trace_seq_printf(p, "%s lba=%llu alloc_len=%u", cmd, lba, alloc_len);
 
 out:
 	trace_seq_putc(p, 0);
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 7/9] arm/ecard: Use get_unaligned_le{16,24}()

[Bart Van Assche]

Use these functions instead of open-coding them.

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/arm/mach-rpc/ecard.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-rpc/ecard.c b/arch/arm/mach-rpc/ecard.c
index 75cfad2cb143..4db4ef085fcb 100644
--- a/arch/arm/mach-rpc/ecard.c
+++ b/arch/arm/mach-rpc/ecard.c
@@ -89,16 +89,6 @@ ecard_loader_reset(unsigned long base, loader_t loader);
 asmlinkage extern int
 ecard_loader_read(int off, unsigned long base, loader_t loader);
 
-static inline unsigned short ecard_getu16(unsigned char *v)
-{
-	return v[0] | v[1] << 8;
-}
-
-static inline signed long ecard_gets24(unsigned char *v)
-{
-	return v[0] | v[1] << 8 | v[2] << 16 | ((v[2] & 0x80) ? 0xff000000 : 0);
-}
-
 static inline ecard_t *slot_to_ecard(unsigned int slot)
 {
 	return slot < MAX_ECARDS ? slot_to_expcard[slot] : NULL;
@@ -915,13 +905,13 @@ static int __init ecard_probe(int slot, unsigned irq, card_type_t type)
 	ec->cid.cd	= cid.r_cd;
 	ec->cid.is	= cid.r_is;
 	ec->cid.w	= cid.r_w;
-	ec->cid.manufacturer = ecard_getu16(cid.r_manu);
-	ec->cid.product = ecard_getu16(cid.r_prod);
+	ec->cid.manufacturer = get_unaligned_le16(cid.r_manu);
+	ec->cid.product = get_unaligned_le16(cid.r_prod);
 	ec->cid.country = cid.r_country;
 	ec->cid.irqmask = cid.r_irqmask;
-	ec->cid.irqoff  = ecard_gets24(cid.r_irqoff);
+	ec->cid.irqoff  = get_unaligned_le24_sign_extend(cid.r_irqoff);
 	ec->cid.fiqmask = cid.r_fiqmask;
-	ec->cid.fiqoff  = ecard_gets24(cid.r_fiqoff);
+	ec->cid.fiqoff  = get_unaligned_le24_sign_extend(cid.r_fiqoff);
 	ec->fiqaddr	=
 	ec->irqaddr	= addr;
 
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 8/9] IB/qib: Sign extend without triggering implementation-defined behavior

[Bart Van Assche]

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: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Cc: Doug Ledford <dledford@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/hw/qib/qib_rc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/qib/qib_rc.c b/drivers/infiniband/hw/qib/qib_rc.c
index aaf7438258fa..2f1beaab6935 100644
--- a/drivers/infiniband/hw/qib/qib_rc.c
+++ b/drivers/infiniband/hw/qib/qib_rc.c
@@ -566,7 +566,7 @@ int qib_make_rc_req(struct rvt_qp *qp, unsigned long *flags)
 		break;
 	}
 	qp->s_sending_hpsn = bth2;
-	delta = (((int) bth2 - (int) wqe->psn) << 8) >> 8;
+	delta = sign_extend_24_to_32(bth2 - wqe->psn);
 	if (delta && delta % QIB_PSN_CREDIT == 0)
 		bth2 |= IB_BTH_REQ_ACK;
 	if (qp->s_flags & RVT_S_SEND_ONE) {
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH 9/9] ASoC/fsl_spdif: Use put_unaligned_be24() instead of open-coding it

[Bart Van Assche]

This patch makes the code easier to read.

Cc: Timur Tabi <timur@kernel.org>
Cc: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: Xiubo Li <Xiubo.Lee@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 sound/soc/fsl/fsl_spdif.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 7858a5499ac5..8e80ae16f566 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -16,6 +16,7 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/regmap.h>
+#include <asm/unaligned.h>
 
 #include <sound/asoundef.h>
 #include <sound/dmaengine_pcm.h>
@@ -173,9 +174,7 @@ static void spdif_irq_uqrx_full(struct fsl_spdif_priv *spdif_priv, char name)
 	}
 
 	regmap_read(regmap, reg, &val);
-	ctrl->subcode[*pos++] = val >> 16;
-	ctrl->subcode[*pos++] = val >> 8;
-	ctrl->subcode[*pos++] = val;
+	put_unaligned_be24(val, &ctrl->subcode[*pos]);
 }
 
 /* U/Q Channel sync found */
-- 
2.24.0.rc0.303.g954a862665-goog

Re: [PATCH 9/9] ASoC/fsl_spdif: Use put_unaligned_be24() instead of open-coding it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

On Mon, Oct 28, 2019 at 01:07:00PM -0700, Bart Van Assche wrote:
> This patch makes the code easier to read.

I only have this patch from the series but no cover letter, what's the
story with dependencies?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 9/9] ASoC/fsl_spdif: Use put_unaligned_be24() instead of open-coding it

[Bart Van Assche]

On 10/28/19 1:24 PM, Mark Brown wrote:
> On Mon, Oct 28, 2019 at 01:07:00PM -0700, Bart Van Assche wrote:
>> This patch makes the code easier to read.
> 
> I only have this patch from the series but no cover letter, what's the
> story with dependencies?

Hi Mark,

The entire patch series including the cover letter is available on e.g. 
the Lore linux-kernel archive: 
https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/T/#t

Thanks,

Bart.

Re: [PATCH 0/9] Consolidate {get,put}_unaligned_[bl]e24() definitions

[Peter Zijlstra]

On Mon, Oct 28, 2019 at 01:06:51PM -0700, Bart Van Assche wrote:
> Hi Peter,
> 
> This patch series moves the existing {get,put}_unaligned_[bl]e24() definitions
> into include/linux/unaligned/generic.h. This patch series also introduces a function
> for sign-extending 24-bit into 32-bit integers and introduces users for all new
> functions and macros. Please consider this patch series for kernel version v5.5.

While I applaud the effort (and didn't see anything off in a hurry), I do
wonder why you think I should route these patches.

I don't think I've ever touched the unaligned accessors...

Re: [PATCH 0/9] Consolidate {get,put}_unaligned_[bl]e24() definitions

[Bart Van Assche]

On 10/28/19 2:52 PM, Peter Zijlstra wrote:
> On Mon, Oct 28, 2019 at 01:06:51PM -0700, Bart Van Assche wrote:
>> Hi Peter,
>>
>> This patch series moves the existing {get,put}_unaligned_[bl]e24() definitions
>> into include/linux/unaligned/generic.h. This patch series also introduces a function
>> for sign-extending 24-bit into 32-bit integers and introduces users for all new
>> functions and macros. Please consider this patch series for kernel version v5.5.
> 
> While I applaud the effort (and didn't see anything off in a hurry), I do
> wonder why you think I should route these patches.
> 
> I don't think I've ever touched the unaligned accessors...

(+ Andrew Morton)

Hi Peter,

I wasn't sure which kernel maintainer to send this patch series to.

Andrew, I think that the include/linux/unaligned/generic.h header file 
went upstream through your tree in 2008. Can you perhaps recommend me to 
which kernel maintainer I should send this patch series? An archived 
version of this patch series is available at 
https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/T/#t.

Thanks,

Bart.

Re: [PATCH 0/9] Consolidate {get,put}_unaligned_[bl]e24() definitions

[Douglas Gilbert]

On 2019-10-28 4:06 p.m., Bart Van Assche wrote:
> Hi Peter,
> 
> This patch series moves the existing {get,put}_unaligned_[bl]e24() definitions
> into include/linux/unaligned/generic.h. This patch series also introduces a function
> for sign-extending 24-bit into 32-bit integers and introduces users for all new
> functions and macros. Please consider this patch series for kernel version v5.5.

And while you are at it, the sg3_utils user space copy of
include/linux/unaligned/*.h (called sg_unaligned.h) has bindings
for 48 bit operations.

Checking my sg3_utils code, in VPD page 0x83 (mandatory device
identification page) the EUI-64 based 16-byte designator has a
6 byte field (the "vendor specific extension identifier").
Also the SET TIMESTAMP and REPORT TIMESTAMP commands have a 6 byte
timestamp field. There are also some attribute pages associated with
the READ ATTRIBUTE command that have 6 byte fields.


A recent trend with the pages returned by the SCSI LOG SENSE command
is to have (big endian) fields whose length (in bytes) is encoded
in the response. I have this function for those:

/* Returns 0 if 'num_bytes' is less than or equal to 0 or greater than
  * 8 (i.e. sizeof(uint64_t)). Else returns result in uint64_t which is
  * an 8 byte unsigned integer. */
static inline uint64_t sg_get_unaligned_be(int num_bytes, const void *p)

And I can see NVMe code in smartmontools using that one:

nvmeprint.cpp:   jrns["eui64"]["ext_id"] =
                                 sg_get_unaligned_be(5, id_ns.eui64 + 3);


Doug Gilbert


> Thanks,
> 
> Bart.
> 
> Bart Van Assche (9):
>    linux/unaligned/byteshift.h: Remove superfluous casts
>    c6x: Include <linux/unaligned/generic.h> instead of duplicating it
>    treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions
>    drivers/iio: Sign extend without triggering implementation-defined
>      behavior
>    scsi/st: Use get_unaligned_signed_be24()
>    scsi/trace: Use get_unaligned_be*()
>    arm/ecard: Use get_unaligned_le{16,24}()
>    IB/qib: Sign extend without triggering implementation-defined behavior
>    ASoC/fsl_spdif: Use put_unaligned_be24() instead of open-coding it
> 
>   arch/arm/mach-rpc/ecard.c                     |  18 +--
>   arch/c6x/include/asm/unaligned.h              |  65 +--------
>   .../iio/common/st_sensors/st_sensors_core.c   |   7 +-
>   drivers/infiniband/hw/qib/qib_rc.c            |   2 +-
>   drivers/nvme/host/rdma.c                      |   8 --
>   drivers/nvme/target/rdma.c                    |   6 -
>   drivers/scsi/scsi_trace.c                     | 128 ++++++------------
>   drivers/scsi/st.c                             |   4 +-
>   drivers/usb/gadget/function/f_mass_storage.c  |   1 +
>   drivers/usb/gadget/function/storage_common.h  |   5 -
>   include/linux/unaligned/be_byteshift.h        |   6 +-
>   include/linux/unaligned/generic.h             |  44 ++++++
>   include/linux/unaligned/le_byteshift.h        |   6 +-
>   include/target/target_core_backend.h          |   6 -
>   sound/soc/fsl/fsl_spdif.c                     |   5 +-
>   15 files changed, 103 insertions(+), 208 deletions(-)
> 

Re: [PATCH 4/9] drivers/iio: Sign extend without triggering implementation-defined behavior

[Jonathan Cameron]

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);

Re: [PATCH 4/9] drivers/iio: Sign extend without triggering implementation-defined behavior

[Peter Zijlstra]

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)

Re: [PATCH 4/9] drivers/iio: Sign extend without triggering implementation-defined behavior

[Douglas Gilbert]

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)
> 

Re: [PATCH 4/9] drivers/iio: Sign extend without triggering implementation-defined behavior

[Bart Van Assche]

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.

Re: [PATCH 2/9] c6x: Include <linux/unaligned/generic.h> instead of duplicating it

[Bart Van Assche]

On 2019-10-28 13:06, Bart Van Assche wrote:
> Use the generic __{get,put}_unaligned_[bl]e() definitions instead of
> duplicating these. Since a later patch will add more definitions into
> <linux/unaligned/generic.h>, this patch ensures that these definitions
> have to be added only once. See also commit a7f626c1948a ("C6X: headers").
> See also commit 6510d41954dc ("kernel: Move arches to use common unaligned
> access").

Mark and Aurelien, are you the c6x maintainers? If so, please let me
know whether you agree with this patch.

Thanks,

Bart.

Re: [PATCH 2/9] c6x: Include <linux/unaligned/generic.h> instead of duplicating it

[Mark Salter]

On Mon, 2019-10-28 at 13:06 -0700, Bart Van Assche wrote:
> Use the generic __{get,put}_unaligned_[bl]e() definitions instead of
> duplicating these. Since a later patch will add more definitions into
> <linux/unaligned/generic.h>, this patch ensures that these definitions
> have to be added only once. See also commit a7f626c1948a ("C6X: headers").
> See also commit 6510d41954dc ("kernel: Move arches to use common unaligned
> access").
> 
> Cc: Mark Salter <msalter@redhat.com>
> Cc: Aurelien Jacquiot <jacquiot.aurelien@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  arch/c6x/include/asm/unaligned.h | 65 +-------------------------------
>  1 file changed, 1 insertion(+), 64 deletions(-)
> 
> diff --git a/arch/c6x/include/asm/unaligned.h b/arch/c6x/include/asm/unaligned.h
> index b56ba7110f5a..d628cc170564 100644
> --- a/arch/c6x/include/asm/unaligned.h
> +++ b/arch/c6x/include/asm/unaligned.h
> @@ -10,6 +10,7 @@
>  #define _ASM_C6X_UNALIGNED_H
>  
>  #include <linux/swab.h>
> +#include <linux/unaligned/generic.h>
>  
>  /*
>   * The C64x+ can do unaligned word and dword accesses in hardware
> @@ -100,68 +101,4 @@ static inline void put_unaligned64(u64 val, const void *p)
>  
>  #endif
>  
> -/*
> - * Cause a link-time error if we try an unaligned access other than
> - * 1,2,4 or 8 bytes long
> - */
> -extern int __bad_unaligned_access_size(void);
> -
> -#define __get_unaligned_le(ptr) (typeof(*(ptr)))({			\
> -	sizeof(*(ptr)) == 1 ? *(ptr) :					\
> -	  (sizeof(*(ptr)) == 2 ? get_unaligned_le16((ptr)) :		\
> -	     (sizeof(*(ptr)) == 4 ? get_unaligned_le32((ptr)) :		\
> -		(sizeof(*(ptr)) == 8 ? get_unaligned_le64((ptr)) :	\
> -		   __bad_unaligned_access_size())));			\
> -	})
> -
> -#define __get_unaligned_be(ptr) (__force typeof(*(ptr)))({	\
> -	sizeof(*(ptr)) == 1 ? *(ptr) :					\
> -	  (sizeof(*(ptr)) == 2 ? get_unaligned_be16((ptr)) :		\
> -	     (sizeof(*(ptr)) == 4 ? get_unaligned_be32((ptr)) :		\
> -		(sizeof(*(ptr)) == 8 ? get_unaligned_be64((ptr)) :	\
> -		   __bad_unaligned_access_size())));			\
> -	})
> -
> -#define __put_unaligned_le(val, ptr) ({					\
> -	void *__gu_p = (ptr);						\
> -	switch (sizeof(*(ptr))) {					\
> -	case 1:								\
> -		*(u8 *)__gu_p = (__force u8)(val);			\
> -		break;							\
> -	case 2:								\
> -		put_unaligned_le16((__force u16)(val), __gu_p);		\
> -		break;							\
> -	case 4:								\
> -		put_unaligned_le32((__force u32)(val), __gu_p);		\
> -		break;							\
> -	case 8:								\
> -		put_unaligned_le64((__force u64)(val), __gu_p);		\
> -		break;							\
> -	default:							\
> -		__bad_unaligned_access_size();				\
> -		break;							\
> -	}								\
> -	(void)0; })
> -
> -#define __put_unaligned_be(val, ptr) ({					\
> -	void *__gu_p = (ptr);						\
> -	switch (sizeof(*(ptr))) {					\
> -	case 1:								\
> -		*(u8 *)__gu_p = (__force u8)(val);			\
> -		break;							\
> -	case 2:								\
> -		put_unaligned_be16((__force u16)(val), __gu_p);		\
> -		break;							\
> -	case 4:								\
> -		put_unaligned_be32((__force u32)(val), __gu_p);		\
> -		break;							\
> -	case 8:								\
> -		put_unaligned_be64((__force u64)(val), __gu_p);		\
> -		break;							\
> -	default:							\
> -		__bad_unaligned_access_size();				\
> -		break;							\
> -	}								\
> -	(void)0; })
> -
>  #endif /* _ASM_C6X_UNALIGNED_H */

Acked-by: Mark Salter <msalter@redhat.com>