Messages in this thread | | | From | Logan Gunthorpe <> | Date | Tue, 5 Dec 2017 12:11:48 -0700 | Subject | Re: [PATCH v2 01/15] NTB: Rename NTB messaging API methods |
| |
On 05/12/17 10:31 AM, Serge Semin wrote: >>> -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.
I agree with Jon to not make this change. The original interface is better. Making the interface similar to spad_read() has no value especially seeing it makes it less correct. As you mention, *msg can be any value (even -1) so this restricts the values possible message values (making future potential hidden bugs) and removes error information (making debugging more difficult).
I haven't really looked into it but, if anything, it would make sense to make the spad_read function more like this one.
Logan
| |