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

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

[Bart Van Assche]

Hi Martin,

This patch series moves the existing {get,put}_unaligned_[bl]e24() definitions
into include/linux/unaligned/generic.h and also replaces some open-coded
implementations of these functions with calls to these functions. Please
consider this patch series for kernel version v5.7.

Thanks,

Bart.

Changes compared to v1:
- Left out the drivers/iio, arm/ecard, IB/qib and ASoC/fsl_spdif patches.
- Dropped the sign_extend_24_to_32(), get_unaligned_signed_be24() and
  get_unaligned_signed_le24() functions.
- See also
  https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/.

Bart Van Assche (5):
  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
  scsi/st: Use get_unaligned_be24() and sign_extend32()
  scsi/trace: Use get_unaligned_be24()

 arch/c6x/include/asm/unaligned.h             | 65 +-------------------
 drivers/nvme/host/rdma.c                     |  8 ---
 drivers/nvme/target/rdma.c                   |  6 --
 drivers/scsi/scsi_trace.c                    |  6 +-
 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            | 46 ++++++++++++++
 include/linux/unaligned/le_byteshift.h       |  6 +-
 include/target/target_core_backend.h         |  6 --
 11 files changed, 58 insertions(+), 101 deletions(-)

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

[Bart Van Assche]

The C language supports implicitly casting a void pointer into a non-void
pointer. 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)

Re: [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts

[Christoph Hellwig]

On Thu, Mar 12, 2020 at 07:37:14PM -0700, Bart Van Assche wrote:
> The C language supports implicitly casting a void pointer into a non-void
> pointer. 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>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 2/5] 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").

Acked-by: 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 */

[PATCH v2 3/5] 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()
implementation.

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            | 46 ++++++++++++++++++++
 include/target/target_core_backend.h         |  6 ---
 6 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 3e85c5cacefd..2845118e6e40 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 37d262a65877..8fcede75e02a 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..5a0cefda7a13 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,48 @@ extern void __bad_unaligned_access_size(void);
 	}								\
 	(void)0; })
 
+static inline u32 __get_unaligned_be24(const u8 *p)
+{
+	return p[0] << 16 | p[1] << 8 | p[2];
+}
+
+static inline u32 get_unaligned_be24(const void *p)
+{
+	return __get_unaligned_be24(p);
+}
+
+static inline u32 __get_unaligned_le24(const u8 *p)
+{
+	return p[0] | p[1] << 8 | p[2] << 16;
+}
+
+static inline u32 get_unaligned_le24(const void *p)
+{
+	return __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 */

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

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
> 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()
> implementation.

Thank you!
My comments below.

After addressing,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> 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            | 46 ++++++++++++++++++++
>  include/target/target_core_backend.h         |  6 ---
>  6 files changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 3e85c5cacefd..2845118e6e40 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 37d262a65877..8fcede75e02a 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..5a0cefda7a13 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,48 @@ extern void __bad_unaligned_access_size(void);
>  	}								\
>  	(void)0; })
>  
> +static inline u32 __get_unaligned_be24(const u8 *p)
> +{
> +	return p[0] << 16 | p[1] << 8 | p[2];
> +}
> +
> +static inline u32 get_unaligned_be24(const void *p)
> +{
> +	return __get_unaligned_be24(p);
> +}
> +
> +static inline u32 __get_unaligned_le24(const u8 *p)
> +{
> +	return p[0] | p[1] << 8 | p[2] << 16;
> +}
> +
> +static inline u32 get_unaligned_le24(const void *p)
> +{
> +	return __get_unaligned_le24(p);
> +}
> +
> +static inline void __put_unaligned_be24(u32 val, u8 *p)

	const u32 val

> +{
> +	*p++ = val >> 16;
> +	*p++ = val >> 8;
> +	*p++ = val;
> +}
> +

> +static inline void put_unaligned_be24(u32 val, void *p)

Ditto.

> +{
> +	__put_unaligned_be24(val, p);
> +}
> +

> +static inline void __put_unaligned_le24(u32 val, u8 *p)

Ditto.

> +{
> +	*p++ = val;
> +	*p++ = val >> 8;
> +	*p++ = val >> 16;
> +}
> +

> +static inline void put_unaligned_le24(u32 val, void *p)

Ditto.

> +{
> +	__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 */

-- 
With Best Regards,
Andy Shevchenko

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

[Bart Van Assche]

On 2020-03-13 02:15, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
>> +static inline void __put_unaligned_be24(u32 val, u8 *p)
> 
> 	const u32 val

Hi Andy,

Thanks for the review. The above suggestion surprises me: as far as I
can tell almost nobody declares function arguments that are passed by
value as 'const' in the Linux kernel:

$ git grep -nH '(const[^\*,]*,' | wc -l
   1065

That number is negligible compared to the number of function declarations:

$ git grep -nH '(.*);$' | wc -l
2692721

Thanks,

Bart.

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

[Andy Shevchenko]

On Fri, Mar 13, 2020 at 07:54:49AM -0700, Bart Van Assche wrote:
> On 2020-03-13 02:15, Andy Shevchenko wrote:
> > On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
> >> +static inline void __put_unaligned_be24(u32 val, u8 *p)
> > 
> > 	const u32 val
> 
> Hi Andy,
> 
> Thanks for the review. The above suggestion surprises me: as far as I
> can tell almost nobody declares function arguments that are passed by
> value as 'const' in the Linux kernel:
> 
> $ git grep -nH '(const[^\*,]*,' | wc -l
>    1065
> 
> That number is negligible compared to the number of function declarations:
> 
> $ git grep -nH '(.*);$' | wc -l
> 2692721

It's a surprising "argument".
If 100500 do worse it doesn't mean 3000 shouldn't do it better.
And of course first grep is incomplete and second one too broad.

Just for (generic) headers:

$ git grep -n '[a-z_0-9]([^)]*\bconst [^)]\+)' -- include | wc -l
4342

$ git grep -n '[a-z_0-9]([^)]\+)' -- include | wc -l
69672

~6% in headers. I don't think it's negligible.

You have at least two advantages on this:
a) we really don't modify the content of the input value;
b) it will be consistent with the rest of consolidated helpers.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 13, 2020 at 06:33:28PM +0200, Andy Shevchenko wrote:

...

> Just for (generic) headers:
> 
> $ git grep -n '[a-z_0-9]([^)]*\bconst [^)]\+)' -- include | wc -l
> 4342
> 
> $ git grep -n '[a-z_0-9]([^)]\+)' -- include | wc -l
> 69672

Just to point out the above is rough estimation, the real one probably feasible
by using coccinelle.

-- 
With Best Regards,
Andy Shevchenko

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

[Bart Van Assche]

On 2020-03-13 09:33, Andy Shevchenko wrote:
> You have at least two advantages on this:
> a) we really don't modify the content of the input value;
> b) it will be consistent with the rest of consolidated helpers.

(b) is an excellent point. I'm all in favor of consistency so I will add
the 'const' keyword.

Thanks,

Bart.

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

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
> 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()
> implementation.
> 
> 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>

Btw, Greg  gave Ack to my patch, I think it applies here as well (for USB)
because the change is basically the same.

> ---
>  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            | 46 ++++++++++++++++++++
>  include/target/target_core_backend.h         |  6 ---
>  6 files changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 3e85c5cacefd..2845118e6e40 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 37d262a65877..8fcede75e02a 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..5a0cefda7a13 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,48 @@ extern void __bad_unaligned_access_size(void);
>  	}								\
>  	(void)0; })
>  
> +static inline u32 __get_unaligned_be24(const u8 *p)
> +{
> +	return p[0] << 16 | p[1] << 8 | p[2];
> +}
> +
> +static inline u32 get_unaligned_be24(const void *p)
> +{
> +	return __get_unaligned_be24(p);
> +}
> +
> +static inline u32 __get_unaligned_le24(const u8 *p)
> +{
> +	return p[0] | p[1] << 8 | p[2] << 16;
> +}
> +
> +static inline u32 get_unaligned_le24(const void *p)
> +{
> +	return __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 */

-- 
With Best Regards,
Andy Shevchenko

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

[Bart Van Assche]

On 2020-03-13 02:25, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
>> 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()
>> implementation.
>>
>> 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>
> 
> Btw, Greg  gave Ack to my patch, I think it applies here as well (for USB)
> because the change is basically the same.

If nobody complains I will add the following:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # For USB

Thanks,

Bart.

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

[Christoph Hellwig]

On Thu, Mar 12, 2020 at 07:37:16PM -0700, Bart Van Assche wrote:
> 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()
> implementation.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

(although the added consts suggested by Andy look useful to me)

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

[Felipe Balbi]

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

Bart Van Assche <bvanassche@acm.org> writes:

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

for drivers/usb/gadget:

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

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

[PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32()

[Bart Van Assche]

Use these functions instead of open-coding them.

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 393f3019ccac..0f315dadf7e8 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -45,6 +45,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>
@@ -2680,8 +2681,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 = sign_extend32(get_unaligned_be24(&cmd[2]), 23);
 	if (direction)
 		sc = -sc;
 	st_printk(ST_DEB_MSG, STp, "Spacing tape %s over %d %s.\n",

Re: [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32()

[Christoph Hellwig]

On Thu, Mar 12, 2020 at 07:37:17PM -0700, Bart Van Assche wrote:
> @@ -2680,8 +2681,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 = sign_extend32(get_unaligned_be24(&cmd[2]), 23);

Btw, didn't the old code here have undefined behavior if cmd[] is
a u8 and we shift by a larger amount?

Re: [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32()

[Bart Van Assche]

On 2020-03-13 04:08, Christoph Hellwig wrote:
> On Thu, Mar 12, 2020 at 07:37:17PM -0700, Bart Van Assche wrote:
>> @@ -2680,8 +2681,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 = sign_extend32(get_unaligned_be24(&cmd[2]), 23);
> 
> Btw, didn't the old code here have undefined behavior if cmd[] is
> a u8 and we shift by a larger amount?

That's a great question. According to my interpretation of the C
standard the above code is fine:

<quote>
6.5.7 Bitwise shift operators
Syntax
  shift-expression:
    additive-expression
    shift-expression << additive-expression
    shift-expression >> additive-expression
Constraints
Each of the operands shall have integer type.
Semantics
The integer promotions are performed on each of the operands. The type
of the result is that of the promoted left operand. If the value of the
right operand is negative or is greater than or equal to the width of
the promoted left operand, the behavior is undefined. [ ... ]
</quote>

Since the RHS has integer type, I think the above means that cmd[2] and
cmd[3] are promoted to a signed integer before being shifted left. As
far as I know the Linux kernel only supports compilers for which
sizeof(int) >= 4 so I think that the old code did not trigger undefined
behavior.

Bart.

[PATCH v2 5/5] scsi/trace: Use get_unaligned_be24()

[Bart Van Assche]

This makes the SCSI tracing code slightly easier to read.

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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c
index ac35c301c792..41a950075913 100644
--- a/drivers/scsi/scsi_trace.c
+++ b/drivers/scsi/scsi_trace.c
@@ -18,11 +18,9 @@ static const char *
 scsi_trace_rw6(struct trace_seq *p, unsigned char *cdb, int len)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	u32 lba = 0, txlen;
+	u32 lba, txlen;
 
-	lba |= ((cdb[1] & 0x1F) << 16);
-	lba |=  (cdb[2] << 8);
-	lba |=   cdb[3];
+	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)).

Re: [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24()

[Christoph Hellwig]

On Thu, Mar 12, 2020 at 07:37:18PM -0700, Bart Van Assche wrote:
> This makes the SCSI tracing code slightly easier to read.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>