| Message ID | 20200312113941.81162-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] asm-generic: Provide generic {get,put}_unaligned_{l,b}e24() | expand |
On Thu, Mar 12, 2020 at 01:39:41PM +0200, Andy Shevchenko wrote: > There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() > implementation. Provide generic helpers once for all. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/nvme/host/rdma.c | 8 ------- > drivers/nvme/target/rdma.c | 6 ----- > drivers/usb/gadget/function/storage_common.h | 5 ---- For usb: Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Thu, Mar 12, 2020 at 01:39:41PM +0200, Andy Shevchenko wrote: > There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() > implementation. Provide generic helpers once for all. I have a vague memory of Bart sending a patch like this a while ago, adding him to verify my theory. Also is there any good to have this in asm-generic/ vs linux/?
On Thu, Mar 12, 2020 at 03:05:42PM +0100, Christoph Hellwig wrote: > On Thu, Mar 12, 2020 at 01:39:41PM +0200, Andy Shevchenko wrote: > > There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() > > implementation. Provide generic helpers once for all. > > I have a vague memory of Bart sending a patch like this a while ago, > adding him to verify my theory. Thanks! > Also is there any good to have this in asm-generic/ vs linux/? For now on it is least invasive. asm-generic/unaligned is a cross point for all unaligned accessor helpers, since we here do byteshift approach for all possible variants, I don't see any other header suitable. Or are you talking about something like linux/unaligned/24bit.h -> #include <...> here?
On 2020-03-12 04:39, Andy Shevchenko wrote: > There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() > implementation. Provide generic helpers once for all. Hi Andy, Thanks for having done this work. In case you would not yet have noticed the patch series that I posted some time ago but for which I did not have the time to continue working on it, please take a look at https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/. Thanks, Bart.
On 2020-03-12 04:39, Andy Shevchenko wrote: > +static inline u32 get_unaligned_be24(const u8 *buf) > +{ > + return (u32)p[0] << 16 | (u32)p[1] << 8 | (u32)p[2]; > +} The argument is called 'buf' and the function body dereferences a pointer called 'p'. Does this even compile? Bart.
On Thu, Mar 12, 2020 at 08:18:07AM -0700, Bart Van Assche wrote: > On 2020-03-12 04:39, Andy Shevchenko wrote: > > There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() > > implementation. Provide generic helpers once for all. > > Hi Andy, > > Thanks for having done this work. In case you would not yet have noticed > the patch series that I posted some time ago but for which I did not > have the time to continue working on it, please take a look at > https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/. Can you send a new version? Also, consider to use byteshift to avoid this limitation: "Only use get_unaligned_be24() if reading p - 1 is allowed."
On 3/12/20 9:25 AM, Andy Shevchenko wrote: > On Thu, Mar 12, 2020 at 08:18:07AM -0700, Bart Van Assche wrote: >> On 2020-03-12 04:39, Andy Shevchenko wrote: >>> There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() >>> implementation. Provide generic helpers once for all. >> >> Hi Andy, >> >> Thanks for having done this work. In case you would not yet have noticed >> the patch series that I posted some time ago but for which I did not >> have the time to continue working on it, please take a look at >> https://lore.kernel.org/lkml/20191028200700.213753-1-bvanassche@acm.org/. > > Can you send a new version? > > Also, consider to use byteshift to avoid this limitation: > "Only use get_unaligned_be24() if reading p - 1 is allowed." Sure, I will do that and I will also add you to the Cc-list of the patch series. Thanks, Bart.
Bart, > Martin, can I send the second version of my patch series to you or do > you perhaps prefer that I send it to another kernel maintainer? I'm > considering to include the following patches: Happy to take it.
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/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/asm-generic/unaligned.h b/include/asm-generic/unaligned.h index 374c940e9be1..dd9f9695d1ba 100644 --- a/include/asm-generic/unaligned.h +++ b/include/asm-generic/unaligned.h @@ -33,4 +33,29 @@ # error need to define endianess #endif +/* 24-bit unaligned access is special for now, that's why explicitly here */ +static inline u32 get_unaligned_le24(const u8 *p) +{ + return (u32)p[0] | (u32)p[1] << 8 | (u32)p[2] << 16; +} + +static inline void put_unaligned_le24(const u32 val, u8 *p) +{ + *p++ = val; + *p++ = val >> 8; + *p++ = val >> 16; +} + +static inline u32 get_unaligned_be24(const u8 *buf) +{ + return (u32)p[0] << 16 | (u32)p[1] << 8 | (u32)p[2]; +} + +static inline void put_unaligned_be24(const u32 val, u8 *p) +{ + *p++ = val >> 16; + *p++ = val >> 8; + *p++ = val; +} + #endif /* __ASM_GENERIC_UNALIGNED_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 */
There are users in kernel that duplicate {get,put}_unaligned_{l,b}e24() implementation. Provide generic helpers once for all. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/nvme/host/rdma.c | 8 ------- drivers/nvme/target/rdma.c | 6 ----- drivers/usb/gadget/function/storage_common.h | 5 ---- include/asm-generic/unaligned.h | 25 ++++++++++++++++++++ include/target/target_core_backend.h | 6 ----- 5 files changed, 25 insertions(+), 25 deletions(-)