* [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions @ 2020-03-13 2:37 Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts Bart Van Assche ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, 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(-) ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche @ 2020-03-13 2:37 ` Bart Van Assche 2020-03-13 11:05 ` Christoph Hellwig 2020-03-13 2:37 ` [PATCH v2 2/5] c6x: Include <linux/unaligned/generic.h> instead of duplicating it Bart Van Assche ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts 2020-03-13 2:37 ` [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts Bart Van Assche @ 2020-03-13 11:05 ` Christoph Hellwig 0 siblings, 0 replies; 19+ messages in thread From: Christoph Hellwig @ 2020-03-13 11:05 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/5] c6x: Include <linux/unaligned/generic.h> instead of duplicating it 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts Bart Van Assche @ 2020-03-13 2:37 ` Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche ` (2 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, Mark Salter, Aurelien Jacquiot 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 */ ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 2/5] c6x: Include <linux/unaligned/generic.h> instead of duplicating it Bart Van Assche @ 2020-03-13 2:37 ` Bart Van Assche 2020-03-13 9:15 ` Andy Shevchenko ` (3 more replies) 2020-03-13 2:37 ` [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() Bart Van Assche 4 siblings, 4 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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 */ ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche @ 2020-03-13 9:15 ` Andy Shevchenko 2020-03-13 14:54 ` Bart Van Assche 2020-03-13 9:25 ` Andy Shevchenko ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Andy Shevchenko @ 2020-03-13 9:15 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 9:15 ` Andy Shevchenko @ 2020-03-13 14:54 ` Bart Van Assche 2020-03-13 16:33 ` Andy Shevchenko 0 siblings, 1 reply; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 14:54 UTC (permalink / raw) To: Andy Shevchenko Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 14:54 ` Bart Van Assche @ 2020-03-13 16:33 ` Andy Shevchenko 2020-03-13 16:38 ` Andy Shevchenko 2020-03-13 20:04 ` Bart Van Assche 0 siblings, 2 replies; 19+ messages in thread From: Andy Shevchenko @ 2020-03-13 16:33 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 16:33 ` Andy Shevchenko @ 2020-03-13 16:38 ` Andy Shevchenko 2020-03-13 20:04 ` Bart Van Assche 1 sibling, 0 replies; 19+ messages in thread From: Andy Shevchenko @ 2020-03-13 16:38 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 16:33 ` Andy Shevchenko 2020-03-13 16:38 ` Andy Shevchenko @ 2020-03-13 20:04 ` Bart Van Assche 1 sibling, 0 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 20:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 9:15 ` Andy Shevchenko @ 2020-03-13 9:25 ` Andy Shevchenko 2020-03-13 14:56 ` Bart Van Assche 2020-03-13 11:06 ` Christoph Hellwig 2020-03-13 14:44 ` Felipe Balbi 3 siblings, 1 reply; 19+ messages in thread From: Andy Shevchenko @ 2020-03-13 9:25 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 9:25 ` Andy Shevchenko @ 2020-03-13 14:56 ` Bart Van Assche 0 siblings, 0 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 14:56 UTC (permalink / raw) To: Andy Shevchenko Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 9:15 ` Andy Shevchenko 2020-03-13 9:25 ` Andy Shevchenko @ 2020-03-13 11:06 ` Christoph Hellwig 2020-03-13 14:44 ` Felipe Balbi 3 siblings, 0 replies; 19+ messages in thread From: Christoph Hellwig @ 2020-03-13 11:06 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Keith Busch, Sagi Grimberg, Jens Axboe, Felipe Balbi, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton 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) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche ` (2 preceding siblings ...) 2020-03-13 11:06 ` Christoph Hellwig @ 2020-03-13 14:44 ` Felipe Balbi 3 siblings, 0 replies; 19+ messages in thread From: Felipe Balbi @ 2020-03-13 14:44 UTC (permalink / raw) To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, Keith Busch, Sagi Grimberg, Jens Axboe, Harvey Harrison, Ingo Molnar, Thomas Gleixner, H . Peter Anvin, Andrew Morton [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche ` (2 preceding siblings ...) 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche @ 2020-03-13 2:37 ` Bart Van Assche 2020-03-13 11:08 ` Christoph Hellwig 2020-03-13 2:37 ` [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() Bart Van Assche 4 siblings, 1 reply; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, Kai Makisara, James E . J . Bottomley 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", ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() 2020-03-13 2:37 ` [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() Bart Van Assche @ 2020-03-13 11:08 ` Christoph Hellwig 2020-03-13 15:03 ` Bart Van Assche 0 siblings, 1 reply; 19+ messages in thread From: Christoph Hellwig @ 2020-03-13 11:08 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Kai Makisara, James E . J . Bottomley 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() 2020-03-13 11:08 ` Christoph Hellwig @ 2020-03-13 15:03 ` Bart Van Assche 0 siblings, 0 replies; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 15:03 UTC (permalink / raw) To: Christoph Hellwig Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Andy Shevchenko, Greg Kroah-Hartman, Kai Makisara, James E . J . Bottomley 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche ` (3 preceding siblings ...) 2020-03-13 2:37 ` [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() Bart Van Assche @ 2020-03-13 2:37 ` Bart Van Assche 2020-03-13 11:08 ` Christoph Hellwig 4 siblings, 1 reply; 19+ messages in thread From: Bart Van Assche @ 2020-03-13 2:37 UTC (permalink / raw) To: Martin K . Petersen, James E . J . Bottomley Cc: linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, Bart Van Assche, James E . J . Bottomley, Colin Ian King 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)). ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() 2020-03-13 2:37 ` [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() Bart Van Assche @ 2020-03-13 11:08 ` Christoph Hellwig 0 siblings, 0 replies; 19+ messages in thread From: Christoph Hellwig @ 2020-03-13 11:08 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi, Christoph Hellwig, Andy Shevchenko, Greg Kroah-Hartman, James E . J . Bottomley, Colin Ian King 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-03-13 20:04 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 2:37 [PATCH v2 0/5] Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 1/5] linux/unaligned/byteshift.h: Remove superfluous casts Bart Van Assche 2020-03-13 11:05 ` Christoph Hellwig 2020-03-13 2:37 ` [PATCH v2 2/5] c6x: Include <linux/unaligned/generic.h> instead of duplicating it Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 3/5] treewide: Consolidate {get,put}_unaligned_[bl]e24() definitions Bart Van Assche 2020-03-13 9:15 ` Andy Shevchenko 2020-03-13 14:54 ` Bart Van Assche 2020-03-13 16:33 ` Andy Shevchenko 2020-03-13 16:38 ` Andy Shevchenko 2020-03-13 20:04 ` Bart Van Assche 2020-03-13 9:25 ` Andy Shevchenko 2020-03-13 14:56 ` Bart Van Assche 2020-03-13 11:06 ` Christoph Hellwig 2020-03-13 14:44 ` Felipe Balbi 2020-03-13 2:37 ` [PATCH v2 4/5] scsi/st: Use get_unaligned_be24() and sign_extend32() Bart Van Assche 2020-03-13 11:08 ` Christoph Hellwig 2020-03-13 15:03 ` Bart Van Assche 2020-03-13 2:37 ` [PATCH v2 5/5] scsi/trace: Use get_unaligned_be24() Bart Van Assche 2020-03-13 11:08 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.