| Message ID | 1446745208-17733-3-git-send-email-eli@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Nov 5, 2015 at 7:40 PM, Eli Cohen <eli@mellanox.com> wrote: > When an extended verbs is an extension to a legacy verb, the original > functionality is preserved. Hence we do not require each hardware driver > to set the extended capability. This will allow to use the extended verb > in its simple form with drivers that do not support the extended > capability. > > Signed-off-by: Eli Cohen <eli@mellanox.com> > --- > drivers/infiniband/core/uverbs_main.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > index e93ba9125198..1740a03e6ac6 100644 > --- a/drivers/infiniband/core/uverbs_main.c > +++ b/drivers/infiniband/core/uverbs_main.c > @@ -672,6 +672,21 @@ out: > return ev_file; > } > > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command) > +{ > + u64 mask; > + > + if (command <= IB_USER_VERBS_CMD_OPEN_QP) I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't think we need two uverbs_cmd_mask vendor variables. IMHO, a vendor shouldn't care if it's an extended/legacy uverb command. Maybe we should replace uverbs_cmd_mask with a bitmap. > + mask = ib_dev->uverbs_cmd_mask; > + else > + mask = ib_dev->uverbs_ex_cmd_mask; > + > + if (mask & ((u64)1 << command)) > + return 0; > + > + return -1; > +} > + > static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > size_t count, loff_t *pos) > { > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > } > > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > + if (verify_command_mask(ib_dev, command)) { > + ret = -EINVAL; Why did you replace ENOSYS with EINVAL? > + goto out; > + } > > flags = (hdr.command & > IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT; > @@ -721,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > goto out; > } > > - if (!(ib_dev->uverbs_cmd_mask & (1ull << command))) { > - ret = -ENOSYS; > - goto out; > - } > - > if (hdr.in_words * 4 != count) { > ret = -EINVAL; > goto out; > @@ -753,11 +767,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > goto out; > } > > - if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) { > - ret = -ENOSYS; > - goto out; > - } > - > if (count < (sizeof(hdr) + sizeof(ex_hdr))) { > ret = -EINVAL; > goto out; > -- > 2.6.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/11/2015 17:04, Matan Barak wrote: >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, >> > } >> > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; >> > + if (verify_command_mask(ib_dev, command)) { >> > + ret = -EINVAL; > Why did you replace ENOSYS with EINVAL? I think ENOSYS is meant only to represent a system call number that isn't supported. There was a change in checkpatch that now warns about it [1]. I'm not sure the intention was to fix existing uses though. Haggai [1] http://www.gossamer-threads.com/lists/linux/kernel/2148343 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 08, 2015 at 05:04:55PM +0200, Matan Barak wrote: > > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command) > > +{ > > + u64 mask; > > + > > + if (command <= IB_USER_VERBS_CMD_OPEN_QP) > > I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't > think we need two uverbs_cmd_mask vendor variables. > IMHO, a vendor shouldn't care if it's an extended/legacy uverb > command. Maybe we should replace uverbs_cmd_mask with a bitmap. > > > + mask = ib_dev->uverbs_cmd_mask; > > + else > > + mask = ib_dev->uverbs_ex_cmd_mask; > > + > > + if (mask & ((u64)1 << command)) > > + return 0; > > + > > + return -1; > > +} > > + But IB_USER_VERBS_CMD_OPEN_QP is the last legacy verbs and I prefer a more restrictive approach to avoid potentail issues in the future. > > static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > > size_t count, loff_t *pos) > > { > > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > > } > > > > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > > + if (verify_command_mask(ib_dev, command)) { > > + ret = -EINVAL; > > Why did you replace ENOSYS with EINVAL? > Like Haggai mentioned in the other response, checkpatch issues error on this claiming that ENOSYS is reserved to unavailable system calls. Is it applicable only for new implementations I am not sure. I don't have clear preference for either ENOSYS or EINAVL. > > + goto out; > > + } > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 9, 2015 at 5:55 PM, Eli Cohen <eli@mellanox.com> wrote: > On Sun, Nov 08, 2015 at 05:04:55PM +0200, Matan Barak wrote: >> > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command) >> > +{ >> > + u64 mask; >> > + >> > + if (command <= IB_USER_VERBS_CMD_OPEN_QP) >> >> I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't >> think we need two uverbs_cmd_mask vendor variables. >> IMHO, a vendor shouldn't care if it's an extended/legacy uverb >> command. Maybe we should replace uverbs_cmd_mask with a bitmap. >> >> > + mask = ib_dev->uverbs_cmd_mask; >> > + else >> > + mask = ib_dev->uverbs_ex_cmd_mask; >> > + >> > + if (mask & ((u64)1 << command)) >> > + return 0; >> > + >> > + return -1; >> > +} >> > + > > But IB_USER_VERBS_CMD_OPEN_QP is the last legacy verbs and I prefer a > more restrictive approach to avoid potentail issues in the future. > So maybe it's worth adding a IB_USER_VERBS_CMD_LAST_LEGACY_VERB. It looks a bit weird why you explicitly check for IB_USER_VERBS_CMD_OPEN_QP. >> > static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, >> > size_t count, loff_t *pos) >> > { >> > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, >> > } >> > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; >> > + if (verify_command_mask(ib_dev, command)) { >> > + ret = -EINVAL; >> >> Why did you replace ENOSYS with EINVAL? >> > > Like Haggai mentioned in the other response, checkpatch issues error > on this claiming that ENOSYS is reserved to unavailable system calls. > Is it applicable only for new implementations I am not sure. I don't > have clear preference for either ENOSYS or EINAVL. I think it could break old applications: err = extended_verb(the_first_extension_we_added); if (err == ENOSYS) err = legacy_verb(); if (err) return err; Such applications used the first extension (that was added during the addition of the extended verb) and when they realized it's not supported, they dropped to the legacy verb. This change can now cause the return of -EINVAL an early termination with an error. >> > + goto out; >> > + } >> > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2015 at 06:11:49PM +0200, Matan Barak wrote: > > > > Like Haggai mentioned in the other response, checkpatch issues error > > on this claiming that ENOSYS is reserved to unavailable system calls. > > Is it applicable only for new implementations I am not sure. I don't > > have clear preference for either ENOSYS or EINAVL. > > I think it could break old applications: > err = extended_verb(the_first_extension_we_added); > if (err == ENOSYS) > err = legacy_verb(); > if (err) > return err; Can you send a pointer to the code where this could happen? > > Such applications used the first extension (that was added during the > addition of the extended verb) and when they realized it's not > supported, they dropped to the legacy verb. This change can now cause > the return of -EINVAL an early termination with an error. > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 9, 2015 at 6:25 PM, Eli Cohen <eli@mellanox.com> wrote: > On Mon, Nov 09, 2015 at 06:11:49PM +0200, Matan Barak wrote: >> > >> > Like Haggai mentioned in the other response, checkpatch issues error >> > on this claiming that ENOSYS is reserved to unavailable system calls. >> > Is it applicable only for new implementations I am not sure. I don't >> > have clear preference for either ENOSYS or EINAVL. >> >> I think it could break old applications: >> err = extended_verb(the_first_extension_we_added); >> if (err == ENOSYS) >> err = legacy_verb(); >> if (err) >> return err; > > Can you send a pointer to the code where this could happen? > This is a hypothetical application code that could break. >> >> Such applications used the first extension (that was added during the >> addition of the extended verb) and when they realized it's not >> supported, they dropped to the legacy verb. This change can now cause >> the return of -EINVAL an early termination with an error. >> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote: > On 08/11/2015 17:04, Matan Barak wrote: > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > >> > } > >> > > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > >> > + if (verify_command_mask(ib_dev, command)) { > >> > + ret = -EINVAL; > > Why did you replace ENOSYS with EINVAL? > > I think ENOSYS is meant only to represent a system call number that > isn't supported. There was a change in checkpatch that now warns about > it [1]. I'm not sure the intention was to fix existing uses though. Within verbs we should have two kinds of return - not supported request, and supported request with invalid parameters. Maybe use EOPNOTSUPP ? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote: > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote: > > On 08/11/2015 17:04, Matan Barak wrote: > > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > > >> > } > > >> > > > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > > >> > + if (verify_command_mask(ib_dev, command)) { > > >> > + ret = -EINVAL; > > > Why did you replace ENOSYS with EINVAL? > > > > I think ENOSYS is meant only to represent a system call number that > > isn't supported. There was a change in checkpatch that now warns about > > it [1]. I'm not sure the intention was to fix existing uses though. > > Within verbs we should have two kinds of return - not supported > request, and supported request with invalid parameters. > > Maybe use EOPNOTSUPP ? > What about Matan's concern regarding legacy code? Maybe we should leave ENOSYS as that's how it is all over the IB stack code. quote: I think it could break old applications: err = extended_verb(the_first_extension_we_added); if (err == ENOSYS) err = legacy_verb(); if (err) return err; Such applications used the first extension (that was added during the addition of the extended verb) and when they realized it's not supported, they dropped to the legacy verb. This change can now cause the return of -EINVAL an early termination with an error. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 01:05:31AM +0200, Eli Cohen wrote: > On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote: > > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote: > > > On 08/11/2015 17:04, Matan Barak wrote: > > > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, > > > >> > } > > > >> > > > > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > > > >> > + if (verify_command_mask(ib_dev, command)) { > > > >> > + ret = -EINVAL; > > > > Why did you replace ENOSYS with EINVAL? > > > > > > I think ENOSYS is meant only to represent a system call number that > > > isn't supported. There was a change in checkpatch that now warns about > > > it [1]. I'm not sure the intention was to fix existing uses though. > > > > Within verbs we should have two kinds of return - not supported > > request, and supported request with invalid parameters. > > > > Maybe use EOPNOTSUPP ? > > > > What about Matan's concern regarding legacy code? Maybe we should > leave ENOSYS as that's how it is all over the IB stack code. > > quote: > I think it could break old applications: > err = extended_verb(the_first_extension_we_added); > if (err == ENOSYS) > err = legacy_verb(); > if (err) > return err; > > Such applications used the first extension (that was added during the > addition of the extended verb) and when they realized it's not > supported, they dropped to the legacy verb. This change can now cause > the return of -EINVAL an early termination with an error. Since the change is to make the kernel do the above fall back internally, this specific example doesn't make alot of sense to worry about. Ie the extended verb won't fail anymore, and if it does the legacy one won't work anyhow. But if there is something out there that does care about ENOSYS we should try to keep it, but don't convert ENOSYS to EINVAL. Also, when the driver tests the ex flags for support it should be returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex stuff could stand a good sanity audit. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2015 at 04:15:42PM -0700, Jason Gunthorpe wrote: > > Since the change is to make the kernel do the above fall back > internally, this specific example doesn't make alot of sense to worry > about. Ie the extended verb won't fail anymore, and if it does the > legacy one won't work anyhow. > Makes sense. > But if there is something out there that does care about ENOSYS we > should try to keep it, but don't convert ENOSYS to EINVAL. > > Also, when the driver tests the ex flags for support it should be > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex > stuff could stand a good sanity audit. > #define EOPNOTSUPP 95 /* Operation not supported on transport endpoint */ This does not seem like an ideal choise either. I think ENOSYS in this case is a better choise. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 01:24:31AM +0200, Eli Cohen wrote: > > Also, when the driver tests the ex flags for support it should be > > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex > > stuff could stand a good sanity audit. > > > > #define EOPNOTSUPP 95 /* Operation not supported on > transport endpoint */ > > This does not seem like an ideal choise either. I think ENOSYS in this > case is a better choise. Call it ENOTSUP then: ENOTSUP Operation not supported (POSIX.1) Same value on Linux. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 1:15 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Tue, Nov 10, 2015 at 01:05:31AM +0200, Eli Cohen wrote: >> On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote: >> > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote: >> > > On 08/11/2015 17:04, Matan Barak wrote: >> > > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, >> > > >> > } >> > > >> > >> > > >> > command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; >> > > >> > + if (verify_command_mask(ib_dev, command)) { >> > > >> > + ret = -EINVAL; >> > > > Why did you replace ENOSYS with EINVAL? >> > > >> > > I think ENOSYS is meant only to represent a system call number that >> > > isn't supported. There was a change in checkpatch that now warns about >> > > it [1]. I'm not sure the intention was to fix existing uses though. >> > >> > Within verbs we should have two kinds of return - not supported >> > request, and supported request with invalid parameters. >> > >> > Maybe use EOPNOTSUPP ? >> > >> >> What about Matan's concern regarding legacy code? Maybe we should >> leave ENOSYS as that's how it is all over the IB stack code. >> >> quote: >> I think it could break old applications: >> err = extended_verb(the_first_extension_we_added); >> if (err == ENOSYS) >> err = legacy_verb(); >> if (err) >> return err; >> >> Such applications used the first extension (that was added during the >> addition of the extended verb) and when they realized it's not >> supported, they dropped to the legacy verb. This change can now cause >> the return of -EINVAL an early termination with an error. > > Since the change is to make the kernel do the above fall back > internally, this specific example doesn't make alot of sense to worry > about. Ie the extended verb won't fail anymore, and if it does the > legacy one won't work anyhow. > The kernel will do the above fallback if the command is a legacy command wrapped in an extended command (i.e - no extra flags). If an application uses one of the extended values, and fall back to legacy verb on ENOSYS, it'll behave differently after this change: Re-posting this example: err = extended_verb(*the_first_extension_we_added*); /* Kernel won't fall back to legacy verbs here, as we use an actual extended request */ if (err == ENOSYS) err = legacy_verb(); if (err) return err; > But if there is something out there that does care about ENOSYS we > should try to keep it, but don't convert ENOSYS to EINVAL. > > Also, when the driver tests the ex flags for support it should be > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex > stuff could stand a good sanity audit. > > Jason Matan -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2015 at 04:30:59PM -0700, Jason Gunthorpe wrote: > On Tue, Nov 10, 2015 at 01:24:31AM +0200, Eli Cohen wrote: > > > Also, when the driver tests the ex flags for support it should be > > > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex > > > stuff could stand a good sanity audit. > > > > > > > #define EOPNOTSUPP 95 /* Operation not supported on > > transport endpoint */ > > > > This does not seem like an ideal choise either. I think ENOSYS in this > > case is a better choise. > > Call it ENOTSUP then: > > ENOTSUP Operation not supported (POSIX.1) > > Same value on Linux. > Yes, that's better. I will resend. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote: > > > > Call it ENOTSUP then: > > > > ENOTSUP Operation not supported (POSIX.1) > > > > Same value on Linux. > > > > Yes, that's better. I will resend. Well it seems like ENOTSUP is defined only here: ./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252 /* Function not implemented (POSIX.4 / HPUX) */ Which obviously I cannot use. Jason, do you have another idea? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 05:40:26PM +0200, Eli Cohen wrote: > On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote: > > > > > > Call it ENOTSUP then: > > > > > > ENOTSUP Operation not supported (POSIX.1) > > > > > > Same value on Linux. > > > > Yes, that's better. I will resend. > > > Well it seems like ENOTSUP is defined only here: > > ./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252 /* Function not implemented (POSIX.4 / HPUX) */ > Which obviously I cannot use. Jason, do you have another idea? I would stick with EOPNOTSUPP in the kernel and userspace can view that as ENOTSUP. My remark was more along the lines that EOPNOTSUPP is not socket specific because it encompasses ENOTSUP as well on Linux, due to having the same value. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 12:25:41PM +0200, Matan Barak wrote: > The kernel will do the above fallback if the command is a legacy > command wrapped in an extended command (i.e - no extra flags). > If an application uses one of the extended values, and fall back to > legacy verb on ENOSYS, it'll behave differently after this change: > Re-posting this example: Again, that doesn't seem like a likely case today, the extended verbs added so far don't strike me as optional, and even so, if someone is coding that kind of fall back it is incorrect to just look for ENOSYS, trying to turn on an optional feature could fail for many different reasons. If someone is actually doing this, then we can work around it, but the kernel seems to be a bit more fluid on error returns - probably because people get them so wrong so often :( Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/2015 07:40 AM, Eli Cohen wrote: > On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote: >>> >>> Call it ENOTSUP then: >>> >>> ENOTSUP Operation not supported (POSIX.1) >>> >>> Same value on Linux. >>> >> >> Yes, that's better. I will resend. > > > Well it seems like ENOTSUP is defined only here: > > ./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252 /* Function not implemented (POSIX.4 / HPUX) */ > > Which obviously I cannot use. Jason, do you have another idea? How about using ENOTSUPP ? $ PAGER= git grep 'define ENOTSUPP' include include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */ Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 11:23:35AM -0800, Bart Van Assche wrote: > > How about using ENOTSUPP ? > > $ PAGER= git grep 'define ENOTSUPP' include > include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */ > Appearently this looks a better option but the following appears as a icomment above this group: /* Defined for the NFSv3 protocol */ I don't mind using this if we can all agree that this is the best choise. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 10:02:26PM +0200, Eli Cohen wrote: > On Tue, Nov 10, 2015 at 11:23:35AM -0800, Bart Van Assche wrote: > > > > How about using ENOTSUPP ? > > > > $ PAGER= git grep 'define ENOTSUPP' include > > include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */ > > > > Appearently this looks a better option but the following appears as a > icomment above this group: /* Defined for the NFSv3 protocol */ > > I don't mind using this if we can all agree that this is the best > choise. We cannot use ENOTSUPP, it is not supported by userspace, there was a list discussion about that a while back. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index e93ba9125198..1740a03e6ac6 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -672,6 +672,21 @@ out: return ev_file; } +static int verify_command_mask(struct ib_device *ib_dev, __u32 command) +{ + u64 mask; + + if (command <= IB_USER_VERBS_CMD_OPEN_QP) + mask = ib_dev->uverbs_cmd_mask; + else + mask = ib_dev->uverbs_ex_cmd_mask; + + if (mask & ((u64)1 << command)) + return 0; + + return -1; +} + static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, size_t count, loff_t *pos) { @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, } command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; + if (verify_command_mask(ib_dev, command)) { + ret = -EINVAL; + goto out; + } flags = (hdr.command & IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT; @@ -721,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } - if (!(ib_dev->uverbs_cmd_mask & (1ull << command))) { - ret = -ENOSYS; - goto out; - } - if (hdr.in_words * 4 != count) { ret = -EINVAL; goto out; @@ -753,11 +767,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } - if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) { - ret = -ENOSYS; - goto out; - } - if (count < (sizeof(hdr) + sizeof(ex_hdr))) { ret = -EINVAL; goto out;
When an extended verbs is an extension to a legacy verb, the original functionality is preserved. Hence we do not require each hardware driver to set the extended capability. This will allow to use the extended verb in its simple form with drivers that do not support the extended capability. Signed-off-by: Eli Cohen <eli@mellanox.com> --- drivers/infiniband/core/uverbs_main.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)