* [PATCH ib-next 0/3] IB core uverbs support for leagacy commands @ 2015-11-05 17:40 Eli Cohen [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-05 17:40 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/, ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, ogerlitz-VPRAkNaXOzVWk0Htik3J/w, eranbe-VPRAkNaXOzVWk0Htik3J/w, matanbe-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen Hi Doug, this patcheset is addresses comments from both Jason and Yann. Eli Eli Cohen (3): IB/core: Avoid duplicate code IB/core: IB/core: Allow legacy verbs through extended interfaces IB/core: Modify conditional on ucontext existence drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 36 deletions(-) -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]
* [PATCH ib-next 1/3] IB/core: Avoid duplicate code [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> @ 2015-11-05 17:40 ` Eli Cohen 2015-11-05 17:40 ` [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces Eli Cohen ` (2 subsequent siblings) 3 siblings, 0 replies; 24+ messages in thread From: Eli Cohen @ 2015-11-05 17:40 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/, ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, ogerlitz-VPRAkNaXOzVWk0Htik3J/w, eranbe-VPRAkNaXOzVWk0Htik3J/w, matanbe-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen Move the check on the validity of the command to a common area. Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> --- drivers/infiniband/core/uverbs_main.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index e3ef28861be6..e93ba9125198 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -678,6 +678,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, struct ib_uverbs_file *file = filp->private_data; struct ib_device *ib_dev; struct ib_uverbs_cmd_hdr hdr; + __u32 command; __u32 flags; int srcu_key; ssize_t ret; @@ -696,20 +697,18 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | + IB_USER_VERBS_CMD_COMMAND_MASK)) { + ret = -EINVAL; + goto out; + } + + command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; + flags = (hdr.command & IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT; if (!flags) { - __u32 command; - - if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | - IB_USER_VERBS_CMD_COMMAND_MASK)) { - ret = -EINVAL; - goto out; - } - - command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; - if (command >= ARRAY_SIZE(uverbs_cmd_table) || !uverbs_cmd_table[command]) { ret = -EINVAL; @@ -738,21 +737,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, hdr.out_words * 4); } else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) { - __u32 command; - struct ib_uverbs_ex_cmd_hdr ex_hdr; struct ib_udata ucore; struct ib_udata uhw; size_t written_count = count; - if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | - IB_USER_VERBS_CMD_COMMAND_MASK)) { - ret = -EINVAL; - goto out; - } - - command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; - if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) || !uverbs_ex_cmd_table[command]) { ret = -ENOSYS; -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-05 17:40 ` [PATCH ib-next 1/3] IB/core: Avoid duplicate code Eli Cohen @ 2015-11-05 17:40 ` Eli Cohen [not found] ` <1446745208-17733-3-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-05 17:40 ` [PATCH ib-next 3/3] IB/core: Modify conditional on ucontext existence Eli Cohen 2015-11-19 21:56 ` [PATCH ib-next 0/3] IB core uverbs support for leagacy commands Eli Cohen 3 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-05 17:40 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/, ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, ogerlitz-VPRAkNaXOzVWk0Htik3J/w, eranbe-VPRAkNaXOzVWk0Htik3J/w, matanbe-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> --- 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) + 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; -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 24+ messages in thread
[parent not found: <1446745208-17733-3-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <1446745208-17733-3-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> @ 2015-11-08 15:04 ` Matan Barak [not found] ` <CAAKD3BDq_o+J3yM1qAKg_x35Dps5Ne_ffaD_Mz55a272pwQcJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Matan Barak @ 2015-11-08 15:04 UTC (permalink / raw) To: Eli Cohen Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w On Thu, Nov 5, 2015 at 7:40 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> > --- > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <CAAKD3BDq_o+J3yM1qAKg_x35Dps5Ne_ffaD_Mz55a272pwQcJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <CAAKD3BDq_o+J3yM1qAKg_x35Dps5Ne_ffaD_Mz55a272pwQcJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-11-09 6:48 ` Haggai Eran [not found] ` <564041C4.1090303-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-09 15:55 ` Eli Cohen 1 sibling, 1 reply; 24+ messages in thread From: Haggai Eran @ 2015-11-09 6:48 UTC (permalink / raw) To: Matan Barak, Eli Cohen Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <564041C4.1090303-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <564041C4.1090303-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> @ 2015-11-09 22:35 ` Jason Gunthorpe [not found] ` <20151109223531.GB1305-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-09 22:35 UTC (permalink / raw) To: Haggai Eran Cc: Matan Barak, Eli Cohen, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109223531.GB1305-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109223531.GB1305-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> @ 2015-11-09 23:05 ` Eli Cohen [not found] ` <20151109230531.GF20103-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-09 23:05 UTC (permalink / raw) To: Jason Gunthorpe Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109230531.GF20103-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109230531.GF20103-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-09 23:15 ` Jason Gunthorpe [not found] ` <20151109231542.GA20707-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-09 23:15 UTC (permalink / raw) To: Eli Cohen Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109231542.GA20707-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109231542.GA20707-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> @ 2015-11-09 23:24 ` Eli Cohen [not found] ` <20151109232431.GA114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-10 10:25 ` Matan Barak 1 sibling, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-09 23:24 UTC (permalink / raw) To: Jason Gunthorpe Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109232431.GA114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109232431.GA114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-09 23:30 ` Jason Gunthorpe [not found] ` <20151109233059.GA21475-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-09 23:30 UTC (permalink / raw) To: Eli Cohen Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109233059.GA21475-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109233059.GA21475-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> @ 2015-11-10 15:23 ` Eli Cohen [not found] ` <20151110152310.GB114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-10 15:23 UTC (permalink / raw) To: Jason Gunthorpe Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151110152310.GB114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151110152310.GB114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-10 15:40 ` Eli Cohen [not found] ` <20151110154026.GC114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-10 15:40 UTC (permalink / raw) To: Jason Gunthorpe Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151110154026.GC114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151110154026.GC114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-10 17:28 ` Jason Gunthorpe 2015-11-10 19:23 ` Bart Van Assche 1 sibling, 0 replies; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-10 17:28 UTC (permalink / raw) To: Eli Cohen Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151110154026.GC114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-10 17:28 ` Jason Gunthorpe @ 2015-11-10 19:23 ` Bart Van Assche [not found] ` <56424437.9090908-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org> 1 sibling, 1 reply; 24+ messages in thread From: Bart Van Assche @ 2015-11-10 19:23 UTC (permalink / raw) To: Eli Cohen, Jason Gunthorpe Cc: Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <56424437.9090908-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <56424437.9090908-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org> @ 2015-11-10 20:02 ` Eli Cohen [not found] ` <20151110200226.GB21849-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-10 20:02 UTC (permalink / raw) To: Bart Van Assche Cc: Jason Gunthorpe, Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151110200226.GB21849-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151110200226.GB21849-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-10 20:10 ` Jason Gunthorpe 0 siblings, 0 replies; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-10 20:10 UTC (permalink / raw) To: Eli Cohen Cc: Bart Van Assche, Haggai Eran, Matan Barak, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109231542.GA20707-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2015-11-09 23:24 ` Eli Cohen @ 2015-11-10 10:25 ` Matan Barak [not found] ` <CAAKD3BAkJiYV8mHUUqAwPZwCsyRR7579+jUC4idGy6snM6rbHA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 24+ messages in thread From: Matan Barak @ 2015-11-10 10:25 UTC (permalink / raw) To: Jason Gunthorpe Cc: Eli Cohen, Haggai Eran, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w On Tue, Nov 10, 2015 at 1:15 AM, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <CAAKD3BAkJiYV8mHUUqAwPZwCsyRR7579+jUC4idGy6snM6rbHA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <CAAKD3BAkJiYV8mHUUqAwPZwCsyRR7579+jUC4idGy6snM6rbHA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-11-10 17:39 ` Jason Gunthorpe 0 siblings, 0 replies; 24+ messages in thread From: Jason Gunthorpe @ 2015-11-10 17:39 UTC (permalink / raw) To: Matan Barak Cc: Eli Cohen, Haggai Eran, Doug Ledford, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <CAAKD3BDq_o+J3yM1qAKg_x35Dps5Ne_ffaD_Mz55a272pwQcJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-09 6:48 ` Haggai Eran @ 2015-11-09 15:55 ` Eli Cohen [not found] ` <20151109155539.GA116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 1 sibling, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-09 15:55 UTC (permalink / raw) To: Matan Barak Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109155539.GA116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109155539.GA116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-09 16:11 ` Matan Barak [not found] ` <CAAKD3BAJi0XSU1VW10jogaHL7wYT6fcDSw=q1Zpu+4+Mf760nQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Matan Barak @ 2015-11-09 16:11 UTC (permalink / raw) To: Eli Cohen Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w On Mon, Nov 9, 2015 at 5:55 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <CAAKD3BAJi0XSU1VW10jogaHL7wYT6fcDSw=q1Zpu+4+Mf760nQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <CAAKD3BAJi0XSU1VW10jogaHL7wYT6fcDSw=q1Zpu+4+Mf760nQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-11-09 16:25 ` Eli Cohen [not found] ` <20151109162513.GB116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 0 siblings, 1 reply; 24+ messages in thread From: Eli Cohen @ 2015-11-09 16:25 UTC (permalink / raw) To: Matan Barak Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20151109162513.GB116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org>]
* Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces [not found] ` <20151109162513.GB116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> @ 2015-11-09 16:29 ` Matan Barak 0 siblings, 0 replies; 24+ messages in thread From: Matan Barak @ 2015-11-09 16:29 UTC (permalink / raw) To: Eli Cohen Cc: Doug Ledford, Jason Gunthorpe, Yann Droneaud, linux-rdma, Or Gerlitz, Eran Ben Elisha, matanbe-VPRAkNaXOzVWk0Htik3J/w On Mon, Nov 9, 2015 at 6:25 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH ib-next 3/3] IB/core: Modify conditional on ucontext existence [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-05 17:40 ` [PATCH ib-next 1/3] IB/core: Avoid duplicate code Eli Cohen 2015-11-05 17:40 ` [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces Eli Cohen @ 2015-11-05 17:40 ` Eli Cohen 2015-11-19 21:56 ` [PATCH ib-next 0/3] IB core uverbs support for leagacy commands Eli Cohen 3 siblings, 0 replies; 24+ messages in thread From: Eli Cohen @ 2015-11-05 17:40 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/, ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, ogerlitz-VPRAkNaXOzVWk0Htik3J/w, eranbe-VPRAkNaXOzVWk0Htik3J/w, matanbe-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen Since we allow to call legacy verbs using their extended counterpart, the check on ucontext has to move up to a common area in case this verb is ever extended. Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> --- drivers/infiniband/core/uverbs_main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 1740a03e6ac6..1c31324af7e4 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -724,6 +724,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } + if (!file->ucontext && + command != IB_USER_VERBS_CMD_GET_CONTEXT) { + ret = -EINVAL; + goto out; + } + flags = (hdr.command & IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT; @@ -734,12 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } - if (!file->ucontext && - command != IB_USER_VERBS_CMD_GET_CONTEXT) { - ret = -EINVAL; - goto out; - } - if (hdr.in_words * 4 != count) { 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH ib-next 0/3] IB core uverbs support for leagacy commands [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> ` (2 preceding siblings ...) 2015-11-05 17:40 ` [PATCH ib-next 3/3] IB/core: Modify conditional on ucontext existence Eli Cohen @ 2015-11-19 21:56 ` Eli Cohen 3 siblings, 0 replies; 24+ messages in thread From: Eli Cohen @ 2015-11-19 21:56 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, ogerlitz-VPRAkNaXOzVWk0Htik3J/w, eranbe-VPRAkNaXOzVWk0Htik3J/w, matanbe-VPRAkNaXOzVWk0Htik3J/w On Thu, Nov 05, 2015 at 07:40:05PM +0200, Eli Cohen wrote: > Hi Doug, > > this patcheset is addresses comments from both Jason and Yann. > > Eli > > Eli Cohen (3): > IB/core: Avoid duplicate code > IB/core: IB/core: Allow legacy verbs through extended interfaces > IB/core: Modify conditional on ucontext existence > > drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------ > 1 file changed, 34 insertions(+), 36 deletions(-) > Hi Doug, this patch set has been reviewe the community and we came to an agreement. When are you planning to apply them. We have pending fucntionality that we want to push which relies on them. Moreover, there have been patches for libibverbs sent by Eran Ben-Elisha a while ago which were also sent a while ago. We need those so I can update libmlx5 as well and have new functionality available. Thanks, Eli -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-11-19 21:56 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-05 17:40 [PATCH ib-next 0/3] IB core uverbs support for leagacy commands Eli Cohen [not found] ` <1446745208-17733-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-05 17:40 ` [PATCH ib-next 1/3] IB/core: Avoid duplicate code Eli Cohen 2015-11-05 17:40 ` [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces Eli Cohen [not found] ` <1446745208-17733-3-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-08 15:04 ` Matan Barak [not found] ` <CAAKD3BDq_o+J3yM1qAKg_x35Dps5Ne_ffaD_Mz55a272pwQcJg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-09 6:48 ` Haggai Eran [not found] ` <564041C4.1090303-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2015-11-09 22:35 ` Jason Gunthorpe [not found] ` <20151109223531.GB1305-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2015-11-09 23:05 ` Eli Cohen [not found] ` <20151109230531.GF20103-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-09 23:15 ` Jason Gunthorpe [not found] ` <20151109231542.GA20707-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2015-11-09 23:24 ` Eli Cohen [not found] ` <20151109232431.GA114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-09 23:30 ` Jason Gunthorpe [not found] ` <20151109233059.GA21475-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2015-11-10 15:23 ` Eli Cohen [not found] ` <20151110152310.GB114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-10 15:40 ` Eli Cohen [not found] ` <20151110154026.GC114170-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-10 17:28 ` Jason Gunthorpe 2015-11-10 19:23 ` Bart Van Assche [not found] ` <56424437.9090908-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org> 2015-11-10 20:02 ` Eli Cohen [not found] ` <20151110200226.GB21849-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-10 20:10 ` Jason Gunthorpe 2015-11-10 10:25 ` Matan Barak [not found] ` <CAAKD3BAkJiYV8mHUUqAwPZwCsyRR7579+jUC4idGy6snM6rbHA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-10 17:39 ` Jason Gunthorpe 2015-11-09 15:55 ` Eli Cohen [not found] ` <20151109155539.GA116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-09 16:11 ` Matan Barak [not found] ` <CAAKD3BAJi0XSU1VW10jogaHL7wYT6fcDSw=q1Zpu+4+Mf760nQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-09 16:25 ` Eli Cohen [not found] ` <20151109162513.GB116821-lgQlq6cFzJSjLWYaRI30zHI+JuX82XLG@public.gmane.org> 2015-11-09 16:29 ` Matan Barak 2015-11-05 17:40 ` [PATCH ib-next 3/3] IB/core: Modify conditional on ucontext existence Eli Cohen 2015-11-19 21:56 ` [PATCH ib-next 0/3] IB core uverbs support for leagacy commands Eli Cohen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.