From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-x242.google.com (mail-vk0-x242.google.com. [2607:f8b0:400c:c05::242]) by gmr-mx.google.com with ESMTPS id f15si50435plr.1.2017.12.05.08.49.12 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 05 Dec 2017 08:49:12 -0800 (PST) Received: by mail-vk0-x242.google.com with SMTP id w190so649471vkd.7 for ; Tue, 05 Dec 2017 08:49:12 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20171203191736.3399-2-fancer.lancer@gmail.com> References: <20171203191736.3399-1-fancer.lancer@gmail.com> <20171203191736.3399-2-fancer.lancer@gmail.com> From: Jon Mason Date: Tue, 5 Dec 2017 11:49:10 -0500 Message-ID: Subject: Re: [PATCH v2 01/15] NTB: Rename NTB messaging API methods Content-Type: text/plain; charset="UTF-8" To: Serge Semin Cc: Dave Jiang , "Hubbe, Allen" , "S-k, Shyam-sundar" , "Yu, Xiangliang" , Gary R Hook , Sergey.Semin@t-platforms.ru, linux-ntb , linux-kernel List-ID: On Sun, Dec 3, 2017 at 2:17 PM, Serge Semin wrote: > There is a common methods signature form used over all the NTB API > like functions naming scheme, arguments names and order, etc. > Recently added NTB messaging API IO callbacks were named a bit > different so should be renamed to be in compliance with the rest > of the API. The changes are made in a way so the developers won't > be able to compile their code without being informed by the compiler. > > Signed-off-by: Serge Semin > --- > drivers/ntb/hw/idt/ntb_hw_idt.c | 27 ++++++++++++--------------- > include/linux/ntb.h | 34 ++++++++++++++++------------------ > 2 files changed, 28 insertions(+), 33 deletions(-) > > diff --git a/drivers/ntb/hw/idt/ntb_hw_idt.c b/drivers/ntb/hw/idt/ntb_hw_idt.c > index 0cd79f367f7c..6fb87c0f0d97 100644 > --- a/drivers/ntb/hw/idt/ntb_hw_idt.c > +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c > @@ -1744,20 +1744,19 @@ static int idt_ntb_msg_clear_mask(struct ntb_dev *ntb, u64 mask_bits) > * idt_ntb_msg_read() - read message register with specified index > * (NTB API callback) > * @ntb: NTB device context. > - * @midx: Message register index > * @pidx: OUT - Port index of peer device a message retrieved from > - * @msg: OUT - Data > + * @midx: Message register index > * > * Read data from the specified message register and source register. > * > - * Return: zero on success, negative error if invalid argument passed. > + * Return: inbound message register value. > */ > -static int idt_ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, u32 *msg) > +static u32 idt_ntb_msg_read(struct ntb_dev *ntb, int *pidx, int midx) > { > struct idt_ntb_dev *ndev = to_ndev_ntb(ntb); > > if (midx < 0 || IDT_MSG_CNT <= midx) > - return -EINVAL; > + return (u32)-1; Please don't do this. If this is an error return standard error number. And why are we casting to an unsigned int now? > > /* Retrieve source port index of the message */ > if (pidx != NULL) { > @@ -1772,18 +1771,15 @@ static int idt_ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, u32 *msg) > } > > /* Retrieve data of the corresponding message register */ > - if (msg != NULL) > - *msg = idt_nt_read(ndev, ntdata_tbl.msgs[midx].in); > - > - return 0; > + return idt_nt_read(ndev, ntdata_tbl.msgs[midx].in); > } > > /* > - * idt_ntb_msg_write() - write data to the specified message register > - * (NTB API callback) > + * idt_ntb_peer_msg_write() - write data to the specified message register > + * (NTB API callback) > * @ntb: NTB device context. > - * @midx: Message register index > * @pidx: Port index of peer device a message being sent to > + * @midx: Message register index > * @msg: Data to send > * > * Just try to send data to a peer. Message status register should be > @@ -1791,7 +1787,8 @@ static int idt_ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, u32 *msg) > * > * Return: zero on success, negative error if invalid argument passed. > */ > -static int idt_ntb_msg_write(struct ntb_dev *ntb, int midx, int pidx, u32 msg) > +static int idt_ntb_peer_msg_write(struct ntb_dev *ntb, int pidx, int midx, > + u32 msg) > { > struct idt_ntb_dev *ndev = to_ndev_ntb(ntb); > unsigned long irqflags; > @@ -2058,7 +2055,7 @@ static const struct ntb_dev_ops idt_ntb_ops = { > .msg_set_mask = idt_ntb_msg_set_mask, > .msg_clear_mask = idt_ntb_msg_clear_mask, > .msg_read = idt_ntb_msg_read, > - .msg_write = idt_ntb_msg_write > + .peer_msg_write = idt_ntb_peer_msg_write > }; > > /* > @@ -2269,7 +2266,7 @@ static ssize_t idt_dbgfs_info_read(struct file *filp, char __user *ubuf, > "Message data:\n"); > for (idx = 0; idx < IDT_MSG_CNT; idx++) { > int src; > - (void)idt_ntb_msg_read(&ndev->ntb, idx, &src, &data); > + data = idt_ntb_msg_read(&ndev->ntb, &src, idx); > off += scnprintf(strbuf + off, size - off, > "\t%hhu. 0x%08x from peer %hhu (Port %hhu)\n", > idx, data, src, ndev->peers[src].port); > diff --git a/include/linux/ntb.h b/include/linux/ntb.h > index c308964777eb..c1646f2c6344 100644 > --- a/include/linux/ntb.h > +++ b/include/linux/ntb.h > @@ -250,7 +250,7 @@ static inline int ntb_ctx_ops_is_valid(const struct ntb_ctx_ops *ops) > * @msg_set_mask: See ntb_msg_set_mask(). > * @msg_clear_mask: See ntb_msg_clear_mask(). > * @msg_read: See ntb_msg_read(). > - * @msg_write: See ntb_msg_write(). > + * @peer_msg_write: See ntb_peer_msg_write(). > */ > struct ntb_dev_ops { > int (*port_number)(struct ntb_dev *ntb); > @@ -321,8 +321,8 @@ struct ntb_dev_ops { > int (*msg_clear_sts)(struct ntb_dev *ntb, u64 sts_bits); > int (*msg_set_mask)(struct ntb_dev *ntb, u64 mask_bits); > int (*msg_clear_mask)(struct ntb_dev *ntb, u64 mask_bits); > - int (*msg_read)(struct ntb_dev *ntb, int midx, int *pidx, u32 *msg); > - int (*msg_write)(struct ntb_dev *ntb, int midx, int pidx, u32 msg); > + u32 (*msg_read)(struct ntb_dev *ntb, int *pidx, int midx); > + int (*peer_msg_write)(struct ntb_dev *ntb, int pidx, int midx, u32 msg); > }; > > static inline int ntb_dev_ops_is_valid(const struct ntb_dev_ops *ops) > @@ -384,7 +384,7 @@ static inline int ntb_dev_ops_is_valid(const struct ntb_dev_ops *ops) > /* !ops->msg_set_mask == !ops->msg_count && */ > /* !ops->msg_clear_mask == !ops->msg_count && */ > !ops->msg_read == !ops->msg_count && > - !ops->msg_write == !ops->msg_count && > + !ops->peer_msg_write == !ops->msg_count && > 1; > } > > @@ -1459,31 +1459,29 @@ static inline int ntb_msg_clear_mask(struct ntb_dev *ntb, u64 mask_bits) > } > > /** > - * ntb_msg_read() - read message register with specified index > + * ntb_msg_read() - read inbound message register with specified index > * @ntb: NTB device context. > - * @midx: Message register index > * @pidx: OUT - Port index of peer device a message retrieved from > - * @msg: OUT - Data > + * @midx: Message register index > * > * Read data from the specified message register. Source port index of a > * message is retrieved as well. > * > - * Return: Zero on success, otherwise a negative error number. > + * Return: The value of the inbound message register. > */ > -static inline int ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, > - u32 *msg) > +static inline u32 ntb_msg_read(struct ntb_dev *ntb, int *pidx, int midx) > { > if (!ntb->ops->msg_read) > - return -EINVAL; > + return ~(u32)0; Same here but in a different way. Please revert this. > > - return ntb->ops->msg_read(ntb, midx, pidx, msg); > + return ntb->ops->msg_read(ntb, pidx, midx); > } > > /** > - * ntb_msg_write() - write data to the specified message register > + * ntb_peer_msg_write() - write data to the specified peer message register > * @ntb: NTB device context. > - * @midx: Message register index > * @pidx: Port index of peer device a message being sent to > + * @midx: Message register index > * @msg: Data to send > * > * Send data to a specified peer device using the defined message register. > @@ -1492,13 +1490,13 @@ static inline int ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, > * > * Return: Zero on success, otherwise a negative error number. > */ > -static inline int ntb_msg_write(struct ntb_dev *ntb, int midx, int pidx, > - u32 msg) > +static inline int ntb_peer_msg_write(struct ntb_dev *ntb, int pidx, int midx, > + u32 msg) > { > - if (!ntb->ops->msg_write) > + if (!ntb->ops->peer_msg_write) > return -EINVAL; > > - return ntb->ops->msg_write(ntb, midx, pidx, msg); > + return ntb->ops->peer_msg_write(ntb, pidx, midx, msg); > } > > #endif > -- > 2.12.0 >