Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753014AbdLESHd (ORCPT ); Tue, 5 Dec 2017 13:07:33 -0500 Received: from mail-vk0-f65.google.com ([209.85.213.65]:35169 "EHLO mail-vk0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752978AbdLESH2 (ORCPT ); Tue, 5 Dec 2017 13:07:28 -0500 X-Google-Smtp-Source: AGs4zMbveyOCpaJfh+yHmhXgk5dpBtsd6qEuehAqjQ/CZUjVWII24aaiU0whmy/eupl8YvZmBBoPPy89GZofCCVhZ8k= MIME-Version: 1.0 In-Reply-To: <20171205173154.GB1701@mobilestation> References: <20171203191736.3399-1-fancer.lancer@gmail.com> <20171203191736.3399-2-fancer.lancer@gmail.com> <20171205173154.GB1701@mobilestation> From: Jon Mason Date: Tue, 5 Dec 2017 13:07:26 -0500 Message-ID: Subject: Re: [PATCH v2 01/15] NTB: Rename NTB messaging API methods 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11051 Lines: 229 On Tue, Dec 5, 2017 at 12:31 PM, Serge Semin wrote: > On Tue, Dec 05, 2017 at 11:49:10AM -0500, Jon Mason wrote: >> 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? >> > > We discussed these changes on the v1 series. Additionally I asked similar > question sometime ago even before the patchset was introduced. > This patch is made to provide the Message interface similar to the Scratchpad > one. I didn't introduce anything new or unjustified. As you can see the > spad_read/peer_spad_read methods return u32 type too. As well as the > Intel/AMD callbacks. The functions are intentionally made to return FFs > in case if some of the passed arguments get out from the allowed limits. > In such circumstances the return value emulates a situation like if user would > reference an invalid PCIe MMIO address. Since the 32-bits register can in general > have any value including -errno ones, then returning an error within the NTB API > would be incorrect. I remember Allen described it this way. > Nobody argued about it last time. If you think it's incorrect, then it should be > changed in both Scratchpad and Message register interfaces. Just because no one said anything before, doesn't mean it's acceptable. I think that is the official Linux code review mantra ;-) Okay, if we are going to do it this way, then return ~0 instead. At least that way there is no ugly cast. > >> > >> > /* 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. >> > > Please see the comment above. > >> > >> > - 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 >> > >> >> -- >> You received this message because you are subscribed to the Google Groups "linux-ntb" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com. >> To post to this group, send email to linux-ntb@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/CAPoiz9xTRToABuuop77j7GcsLaZjYWD1Ght9kGc-Bo9A0w5ddw%40mail.gmail.com. >> For more options, visit https://groups.google.com/d/optout.