| Message ID | 20151104193621.GA30146@x-vnc01.mtx.labs.mlnx (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, Le mercredi 04 novembre 2015 à 21:36 +0200, Eli Cohen a écrit : > 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. > > In addition, avoid code duplication by moving sanity checks to a > common > area. > Those checks were written with the assumption the command and flags masks could be different between the legacy and the new verb format. But since it did happen (yet), it's ok to avoid the duplication (I would have prefer a separate preliminary patch for this). > Change-Id: Iedba714224fa07b85325c146621c07e0dbf349fb > Signed-off-by: Eli Cohen <eli@mellanox.com> > --- > drivers/infiniband/core/uverbs_main.c | 30 ++++++++++--------------- > ----- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs_main.c > b/drivers/infiniband/core/uverbs_main.c > index e3ef28861be6..0ae934d81b04 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; > @@ -699,17 +700,15 @@ static ssize_t ib_uverbs_write(struct file > *filp, const char __user *buf, > 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; > - } > + 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; > + command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; > > + if (!flags) { > 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; > @@ -764,7 +753,8 @@ static ssize_t ib_uverbs_write(struct file *filp, > const char __user *buf, > goto out; > } > > - if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << > command))) { > + if ((command > IB_USER_VERBS_CMD_OPEN_QP) && What about IB_USER_VERBS_CMD_THRESHOLD instead of IB_USER_VERBS_CMD_OPEN_QP ? > + !(ib_dev->uverbs_ex_cmd_mask & (1ull << > command))) { > ret = -ENOSYS; > goto out; > } uverbs_ex_cmd_table[] has already been looked up at this point, so that can only work if uverbs_ex_cmd_table[] is extended to support the legacy verbs. Currently there's only two verbs with dual implementation IB_USER_VERBS_EX_CMD_QUERY_DEVICE IB_USER_VERBS_EX_CMD_CREATE_CQ If we want to have a 1:1 mapping legacy verbs as extended verbs, then I would suggest to check the legacy mask to not allow a legacy verb not supported by the hw provider to be called: if (! (((command <= IB_USER_VERBS_CMD_OPEN_QP) ? ib_dev->uverbs_cmd_mask : ib_dev->uverbs_ex_cmd_mask) & (1 ull << command))) { ret = -ENOSYS; goto out; } Perhaps we could drop uverbs_ex_cmd_mask and set bit for extended verbs in uverbs_cmd_mask instead. Then, there will be no need to check againt the command threshold. Anyway, this is a new assumption that extended verbs will have to be somewhat retro compatible with the legacy hw provider implementation used by the legacy verbs they intend to replace: an extended verbs matching a legacy one will need to be written in such way it will not be calling into a hw provider function pointer that can be NULL (in case there's new function pointer is added for an updated verbs in struct ib_device while we keep in place the legacy one ... but for in -kernel drivers that should never happen: it usual better to replace the previous one by the newer and the existing drivers have to be updated to provide the new function and remove the previous one, so I think it's quite safe). Regards.
On Thu, Nov 05, 2015 at 11:41:57AM +0100, Yann Droneaud wrote: > > Those checks were written with the assumption the command and flags > masks could be different between the legacy and the new verb format. > > But since it did happen (yet), it's ok to avoid the duplication (I > would have prefer a separate preliminary patch for this). OK, will send in a different patch. > > > > What about IB_USER_VERBS_CMD_THRESHOLD instead of > IB_USER_VERBS_CMD_OPEN_QP ? > I wanted to be more restrictive here. After all there can't be any more legacy commands. > > + !(ib_dev->uverbs_ex_cmd_mask & (1ull << > > command))) { > > ret = -ENOSYS; > > goto out; > > } > > > uverbs_ex_cmd_table[] has already been looked up at this point, so that > can only work if uverbs_ex_cmd_table[] is extended to support the > legacy verbs. > > Currently there's only two verbs with dual implementation > > IB_USER_VERBS_EX_CMD_QUERY_DEVICE > IB_USER_VERBS_EX_CMD_CREATE_CQ > > > If we want to have a 1:1 mapping legacy verbs as extended verbs, then I > would suggest to check the legacy mask to not allow a legacy verb not > supported by the hw provider to be called: > > if (! (((command <= IB_USER_VERBS_CMD_OPEN_QP) ? > ib_dev->uverbs_cmd_mask : > ib_dev->uverbs_ex_cmd_mask) & (1 ull << command))) { > ret = -ENOSYS; > goto out; > } > Yes this looks even safer. So I will apply this idea and re-send. > Perhaps we could drop uverbs_ex_cmd_mask and set bit for extended verbs > in uverbs_cmd_mask instead. Then, there will be no need to check againt > the command threshold. > > Anyway, this is a new assumption that extended verbs will have to be > somewhat retro compatible with the legacy hw provider implementation > used by the legacy verbs they intend to replace: an extended verbs > matching a legacy one will need to be written in such way it will not > be calling into a hw provider function pointer that can be NULL (in > case there's new function pointer is added for an updated verbs in > struct ib_device while we keep in place the legacy one ... but for in > -kernel drivers that should never happen: it usual better to replace > the previous one by the newer and the existing drivers have to be > updated to provide the new function and remove the previous one, so I > think it's quite safe). > > Regards. > > -- > Yann Droneaud > OPTEYA > > -- > 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
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index e3ef28861be6..0ae934d81b04 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; @@ -699,17 +700,15 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, 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; - } + 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; + command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; + if (!flags) { 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; @@ -764,7 +753,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, goto out; } - if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) { + if ((command > IB_USER_VERBS_CMD_OPEN_QP) && + !(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) { ret = -ENOSYS; 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. In addition, avoid code duplication by moving sanity checks to a common area. Change-Id: Iedba714224fa07b85325c146621c07e0dbf349fb Signed-off-by: Eli Cohen <eli@mellanox.com> --- drivers/infiniband/core/uverbs_main.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)