| Message ID | 1546951333-8109-2-git-send-email-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB device in-kernel API support indication | expand |
> On Jan 8, 2019, at 2:42 PM, Gal Pressman <galpress@amazon.com> wrote: > > Drivers that do not provide kernel verbs support should not be used by > ib kernel clients and fail. > In case a device does not implement all mandatory verbs for kverbs usage > mark it as a non kverbs provider and prevent its usage for all clients > except for uverbs. > > The device is marked as a non kverbs provider using the > 'kverbs_provider' flag which should only be set by the core code. > The clients can choose whether kverbs are requested for it its > usage using > the 'no_kverbs_req' flag which is currently set for uverbs only. > > This patch allows drivers to remove mandatory verbs stubs and simply set > the callback to NULL. The IB device will be registered as a non-kverbs > provider. > > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/core/device.c | 41 ++++++++++++++++++++++------------- > drivers/infiniband/core/uverbs_main.c | 1 + > drivers/infiniband/core/verbs.c | 28 +++++++++++++++++++++--- > drivers/infiniband/hw/mlx5/main.c | 3 +++ > include/rdma/ib_verbs.h | 5 +++++ > 5 files changed, 60 insertions(+), 18 deletions(-) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 8872453e26c0..156f1b2ebf16 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -121,13 +121,12 @@ static int ib_device_check_mandatory(struct ib_device *device) > }; > int i; > > + device->kverbs_provider = true; > for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { > if (!*(void **) ((void *) &device->ops + > mandatory_table[i].offset)) { > - dev_warn(&device->dev, > - "Device is missing mandatory function %s\n", > - mandatory_table[i].name); > - return -EINVAL; > + device->kverbs_provider = false; > + break; > } > } > > @@ -374,10 +373,12 @@ static int read_port_immutable(struct ib_device *device) > return -ENOMEM; > > for (port = start_port; port <= end_port; ++port) { > - ret = device->ops.get_port_immutable( > - device, port, &device->port_immutable[port]); > - if (ret) > - return ret; > + if (device->ops.get_port_immutable) { > + ret = device->ops.get_port_immutable( > + device, port, &device->port_immutable[port]); > + if (ret) > + return ret; > + } > > if (verify_immutable(device, port)) > return -EINVAL; > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > } > > memset(&device->attrs, 0, sizeof(device->attrs)); > - ret = device->ops.query_device(device, &device->attrs, &uhw); > - if (ret) { > - dev_warn(&device->dev, > - "Couldn't query the device attributes\n"); > - goto port_cleanup; > + if (device->ops.query_device) { Why do we need these kind of checks now? In case device doesn’t implement all mandatory kverbs, then clients won’t add it.. uverbs has uverbs_cmd_mask in write path, and checks function pointer in the ioctl path.. Maybe I’m missing something.. > + ret = device->ops.query_device(device, &device->attrs, &uhw); > + if (ret) { > + dev_warn(&device->dev, > + "Couldn't query the device attributes\n"); > + goto port_cleanup; > + } > } > > ret = setup_port_pkey_list(device); > @@ -624,7 +627,8 @@ int ib_register_device(struct ib_device *device, const char *name, > > list_for_each_entry(client, &client_list, list) > if (!add_client_context(device, client) && client->add) > - client->add(device); > + if (device->kverbs_provider || client->no_kverbs_req) > + client->add(device); > > down_write(&lists_rwsem); > list_add_tail(&device->core_list, &device_list); > @@ -721,7 +725,8 @@ int ib_register_client(struct ib_client *client) > > list_for_each_entry(device, &device_list, core_list) > if (!add_client_context(device, client) && client->add) > - client->add(device); > + if (device->kverbs_provider || client->no_kverbs_req) > + client->add(device); > > down_write(&lists_rwsem); > list_add_tail(&client->list, &client_list); > @@ -920,6 +925,9 @@ int ib_query_port(struct ib_device *device, > union ib_gid gid; > int err; > > + if (!device->ops.query_port) > + return -EOPNOTSUPP; > + > if (!rdma_is_port_valid(device, port_num)) > return -EINVAL; > > @@ -1043,6 +1051,9 @@ int ib_enum_all_devs(nldev_callback nldev_cb, struct sk_buff *skb, > int ib_query_pkey(struct ib_device *device, > u8 port_num, u16 index, u16 *pkey) > { > + if (!device->ops.query_pkey) > + return -EOPNOTSUPP; > + > if (!rdma_is_port_valid(device, port_num)) > return -EINVAL; > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > index fb0007aa0c27..0eafee9a2ffc 100644 > --- a/drivers/infiniband/core/uverbs_main.c > +++ b/drivers/infiniband/core/uverbs_main.c > @@ -1127,6 +1127,7 @@ static const struct file_operations uverbs_mmap_fops = { > > static struct ib_client uverbs_client = { > .name = "uverbs", > + .no_kverbs_req = true, > .add = ib_uverbs_add_one, > .remove = ib_uverbs_remove_one > }; > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index c51e2505a9ad..54be4521c235 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -257,6 +257,10 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags, > struct ib_pd *pd; > int mr_access_flags = 0; > > + if (!device->ops.alloc_pd || > + (mr_access_flags && !pd->device->ops.get_dma_mr)) > + return ERR_PTR(-EOPNOTSUPP); > + > pd = device->ops.alloc_pd(device, NULL, NULL); > if (IS_ERR(pd)) > return pd; > @@ -320,6 +324,10 @@ void ib_dealloc_pd(struct ib_pd *pd) > { > int ret; > > + if (!pd->device->ops.dealloc_pd || > + (pd->__internal_mr && !pd->device->ops.dereg_mr)) > + return; > + > if (pd->__internal_mr) { > ret = pd->device->ops.dereg_mr(pd->__internal_mr); > WARN_ON(ret); > @@ -1124,10 +1132,12 @@ static struct ib_qp *ib_create_xrc_qp(struct ib_qp *qp, > > qp = __ib_open_qp(real_qp, qp_init_attr->event_handler, > qp_init_attr->qp_context); > - if (!IS_ERR(qp)) > + if (!IS_ERR(qp)) { > __ib_insert_xrcd_qp(qp_init_attr->xrcd, real_qp); > - else > - real_qp->device->ops.destroy_qp(real_qp); > + } else { > + if (real_qp->device->ops.destroy_qp) > + real_qp->device->ops.destroy_qp(real_qp); > + } > return qp; > } > > @@ -1604,6 +1614,9 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr, > const struct ib_gid_attr *old_sgid_attr_alt_av; > int ret; > > + if (!qp->device->ops.modify_qp) > + return -EOPNOTSUPP; > + > if (attr_mask & IB_QP_AV) { > ret = rdma_fill_sgid_attr(qp->device, &attr->ah_attr, > &old_sgid_attr_av); > @@ -1841,6 +1854,9 @@ int ib_destroy_qp(struct ib_qp *qp) > struct ib_qp_security *sec; > int ret; > > + if (!qp->device->ops.destroy_qp) > + return -EOPNOTSUPP; > + > WARN_ON_ONCE(qp->mrs_used > 0); > > if (atomic_read(&qp->usecnt)) > @@ -1928,6 +1944,9 @@ EXPORT_SYMBOL(rdma_set_cq_moderation); > > int ib_destroy_cq(struct ib_cq *cq) > { > + if (!cq->device->ops.destroy_cq) > + return -EOPNOTSUPP; > + > if (atomic_read(&cq->usecnt)) > return -EBUSY; > > @@ -1951,6 +1970,9 @@ int ib_dereg_mr(struct ib_mr *mr) > struct ib_dm *dm = mr->dm; > int ret; > > + if (!mr->device->ops.dereg_mr) > + return -EOPNOTSUPP; > + > rdma_restrack_del(&mr->res); > ret = mr->device->ops.dereg_mr(mr); > if (!ret) { > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c > index 94fe253d4956..072fd8be3355 100644 > --- a/drivers/infiniband/hw/mlx5/main.c > +++ b/drivers/infiniband/hw/mlx5/main.c > @@ -149,6 +149,9 @@ static int get_port_state(struct ib_device *ibdev, > struct ib_port_attr attr; > int ret; > > + if (!ibdev->ops.query_port) > + return -EOPNOTSUPP; > + > memset(&attr, 0, sizeof(attr)); > ret = ibdev->ops.query_port(ibdev, port_num, &attr); > if (!ret) > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index c073a4720d28..fb6074d2e394 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -2566,6 +2566,8 @@ struct ib_device { > __be64 node_guid; > u32 local_dma_lkey; > u16 is_switch:1; > + /* Indicates kernel verbs support, should not be used in drivers */ > + u8 kverbs_provider:1; > u8 node_type; > u8 phys_port_cnt; > struct ib_device_attr attrs; > @@ -2620,6 +2622,9 @@ struct ib_client { > const struct sockaddr *addr, > void *client_data); > struct list_head list; > + > + /* kverbs are not required by the client */ > + u8 no_kverbs_req:1; > }; > > struct ib_device *ib_alloc_device(size_t size); > -- > 2.7.4 >
On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > > } > > > > memset(&device->attrs, 0, sizeof(device->attrs)); > > - ret = device->ops.query_device(device, &device->attrs, &uhw); > > - if (ret) { > > - dev_warn(&device->dev, > > - "Couldn't query the device attributes\n"); > > - goto port_cleanup; > > + if (device->ops.query_device) { > Why do we need these kind of checks now? In case device doesn’t > implement all mandatory kverbs, then clients won’t add it.. uverbs > has uverbs_cmd_mask in write path, and checks function pointer in > the ioctl path.. Maybe I’m missing something.. Yah, I agree, the checks should be in the uverbs layer, and presumably they are all there already, it just needs careful checking. Jason
On 09-Jan-19 01:39, Jason Gunthorpe wrote: > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: >>> @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) >>> } >>> >>> memset(&device->attrs, 0, sizeof(device->attrs)); >>> - ret = device->ops.query_device(device, &device->attrs, &uhw); >>> - if (ret) { >>> - dev_warn(&device->dev, >>> - "Couldn't query the device attributes\n"); >>> - goto port_cleanup; >>> + if (device->ops.query_device) { > >> Why do we need these kind of checks now? In case device doesn’t >> implement all mandatory kverbs, then clients won’t add it.. uverbs >> has uverbs_cmd_mask in write path, and checks function pointer in >> the ioctl path.. Maybe I’m missing something.. > > Yah, I agree, the checks should be in the uverbs layer, and presumably > they are all there already, it just needs careful checking. I've inspected each flow that comes down to using these callbacks, the checks are only added for flows that are not already protected by NULL checks (by the uverbs layer or in use by kverbs only which guarantees callback existence). Specifically, the check Majd pointed out in setup_device is called as part of ib_register_device which is not protected by the uverbs layer. Other checks need careful inspection: ib_query_pkey is called from sysfs flow for example, which assumes the callback exists. ib_query_port has many call sites in drivers which assume the callback exist. Create CQ/QP flows are checked for create_qp/cq existence by the uverbs layer, but in case of failure call destroy_qp/cq which are no longer guaranteed to exist. And the list goes on.. Every additional if I added is there since I believe that it's not already covered by the uverbs layer/kverbs usage only. If you find an unnecessary one please let me know so I'll remove it.
On Tue, Jan 08, 2019 at 04:39:21PM -0700, Jason Gunthorpe wrote: > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > > > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > > > } > > > > > > memset(&device->attrs, 0, sizeof(device->attrs)); > > > - ret = device->ops.query_device(device, &device->attrs, &uhw); > > > - if (ret) { > > > - dev_warn(&device->dev, > > > - "Couldn't query the device attributes\n"); > > > - goto port_cleanup; > > > + if (device->ops.query_device) { > > > Why do we need these kind of checks now? In case device doesn’t > > implement all mandatory kverbs, then clients won’t add it.. uverbs > > has uverbs_cmd_mask in write path, and checks function pointer in > > the ioctl path.. Maybe I’m missing something.. > > Yah, I agree, the checks should be in the uverbs layer, and presumably > they are all there already, it just needs careful checking. Didn't we plan to remove uverbs_cmd_mask and move to != NULL checks? > > Jason
On Wed, Jan 09, 2019 at 09:52:14AM +0200, Gal Pressman wrote: > On 09-Jan-19 01:39, Jason Gunthorpe wrote: > > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > >>> @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > >>> } > >>> > >>> memset(&device->attrs, 0, sizeof(device->attrs)); > >>> - ret = device->ops.query_device(device, &device->attrs, &uhw); > >>> - if (ret) { > >>> - dev_warn(&device->dev, > >>> - "Couldn't query the device attributes\n"); > >>> - goto port_cleanup; > >>> + if (device->ops.query_device) { > > > >> Why do we need these kind of checks now? In case device doesn’t > >> implement all mandatory kverbs, then clients won’t add it.. uverbs > >> has uverbs_cmd_mask in write path, and checks function pointer in > >> the ioctl path.. Maybe I’m missing something.. > > > > Yah, I agree, the checks should be in the uverbs layer, and presumably > > they are all there already, it just needs careful checking. > > I've inspected each flow that comes down to using these callbacks, the checks > are only added for flows that are not already protected by NULL checks (by the > uverbs layer or in use by kverbs only which guarantees callback existence). > > Specifically, the check Majd pointed out in setup_device is called as part of > ib_register_device which is not protected by the uverbs layer. OK > Other checks need careful inspection: > ib_query_pkey is called from sysfs flow for example, which assumes the callback > exists. Don't create pkey sysfs files if they can't work > ib_query_port has many call sites in drivers which assume the > callback exist. Presumably a non-kverbs driver will not call itself in a bad way.. > Create CQ/QP flows are checked for create_qp/cq existence by the > uverbs layer, but in case of failure call destroy_qp/cq which are no > longer guaranteed to exist. The CQ and QP objects and all their uAPI methods are deleted if the destroy methods are not available, so I don't think this is real. Jason
On Wed, Jan 09, 2019 at 10:44:56AM +0200, Leon Romanovsky wrote: > On Tue, Jan 08, 2019 at 04:39:21PM -0700, Jason Gunthorpe wrote: > > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > > > > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > > > > } > > > > > > > > memset(&device->attrs, 0, sizeof(device->attrs)); > > > > - ret = device->ops.query_device(device, &device->attrs, &uhw); > > > > - if (ret) { > > > > - dev_warn(&device->dev, > > > > - "Couldn't query the device attributes\n"); > > > > - goto port_cleanup; > > > > + if (device->ops.query_device) { > > > > > Why do we need these kind of checks now? In case device doesn’t > > > implement all mandatory kverbs, then clients won’t add it.. uverbs > > > has uverbs_cmd_mask in write path, and checks function pointer in > > > the ioctl path.. Maybe I’m missing something.. > > > > Yah, I agree, the checks should be in the uverbs layer, and presumably > > they are all there already, it just needs careful checking. > > Didn't we plan to remove uverbs_cmd_mask and move to != NULL checks? The uverbs layer now uses the NULL checks exhaustively, I think. Jason
On Wed, Jan 09, 2019 at 08:32:40AM -0700, Jason Gunthorpe wrote: > On Wed, Jan 09, 2019 at 10:44:56AM +0200, Leon Romanovsky wrote: > > On Tue, Jan 08, 2019 at 04:39:21PM -0700, Jason Gunthorpe wrote: > > > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > > > > > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > > > > > } > > > > > > > > > > memset(&device->attrs, 0, sizeof(device->attrs)); > > > > > - ret = device->ops.query_device(device, &device->attrs, &uhw); > > > > > - if (ret) { > > > > > - dev_warn(&device->dev, > > > > > - "Couldn't query the device attributes\n"); > > > > > - goto port_cleanup; > > > > > + if (device->ops.query_device) { > > > > > > > Why do we need these kind of checks now? In case device doesn’t > > > > implement all mandatory kverbs, then clients won’t add it.. uverbs > > > > has uverbs_cmd_mask in write path, and checks function pointer in > > > > the ioctl path.. Maybe I’m missing something.. > > > > > > Yah, I agree, the checks should be in the uverbs layer, and presumably > > > they are all there already, it just needs careful checking. > > > > Didn't we plan to remove uverbs_cmd_mask and move to != NULL checks? > > The uverbs layer now uses the NULL checks exhaustively, I think. So, what does it mean "Yah, I agree,"? Are you agree to check in uverbs layer and remove uverbs_cmd_mask? Thanks > > Jason
On Wed, Jan 09, 2019 at 05:43:40PM +0200, Leon Romanovsky wrote: > On Wed, Jan 09, 2019 at 08:32:40AM -0700, Jason Gunthorpe wrote: > > On Wed, Jan 09, 2019 at 10:44:56AM +0200, Leon Romanovsky wrote: > > > On Tue, Jan 08, 2019 at 04:39:21PM -0700, Jason Gunthorpe wrote: > > > > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote: > > > > > > @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) > > > > > > } > > > > > > > > > > > > memset(&device->attrs, 0, sizeof(device->attrs)); > > > > > > - ret = device->ops.query_device(device, &device->attrs, &uhw); > > > > > > - if (ret) { > > > > > > - dev_warn(&device->dev, > > > > > > - "Couldn't query the device attributes\n"); > > > > > > - goto port_cleanup; > > > > > > + if (device->ops.query_device) { > > > > > > > > > Why do we need these kind of checks now? In case device doesn’t > > > > > implement all mandatory kverbs, then clients won’t add it.. uverbs > > > > > has uverbs_cmd_mask in write path, and checks function pointer in > > > > > the ioctl path.. Maybe I’m missing something.. > > > > > > > > Yah, I agree, the checks should be in the uverbs layer, and presumably > > > > they are all there already, it just needs careful checking. > > > > > > Didn't we plan to remove uverbs_cmd_mask and move to != NULL checks? > > > > The uverbs layer now uses the NULL checks exhaustively, I think. > > So, what does it mean "Yah, I agree,"? Are you agree to check in uverbs > layer and remove uverbs_cmd_mask? That we should generally rely on existing uverbs checks and the new no-clients model. The cmd_mask and NULL checks should be equivilent right now. Jason
On 09-Jan-19 17:31, Jason Gunthorpe wrote:>> Create CQ/QP flows are checked for create_qp/cq existence by the >> uverbs layer, but in case of failure call destroy_qp/cq which are no >> longer guaranteed to exist. > > The CQ and QP objects and all their uAPI methods are deleted if the > destroy methods are not available, so I don't think this is real. Thanks, I didn't realize that. Will fix and post v4.
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 8872453e26c0..156f1b2ebf16 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -121,13 +121,12 @@ static int ib_device_check_mandatory(struct ib_device *device) }; int i; + device->kverbs_provider = true; for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { if (!*(void **) ((void *) &device->ops + mandatory_table[i].offset)) { - dev_warn(&device->dev, - "Device is missing mandatory function %s\n", - mandatory_table[i].name); - return -EINVAL; + device->kverbs_provider = false; + break; } } @@ -374,10 +373,12 @@ static int read_port_immutable(struct ib_device *device) return -ENOMEM; for (port = start_port; port <= end_port; ++port) { - ret = device->ops.get_port_immutable( - device, port, &device->port_immutable[port]); - if (ret) - return ret; + if (device->ops.get_port_immutable) { + ret = device->ops.get_port_immutable( + device, port, &device->port_immutable[port]); + if (ret) + return ret; + } if (verify_immutable(device, port)) return -EINVAL; @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device) } memset(&device->attrs, 0, sizeof(device->attrs)); - ret = device->ops.query_device(device, &device->attrs, &uhw); - if (ret) { - dev_warn(&device->dev, - "Couldn't query the device attributes\n"); - goto port_cleanup; + if (device->ops.query_device) { + ret = device->ops.query_device(device, &device->attrs, &uhw); + if (ret) { + dev_warn(&device->dev, + "Couldn't query the device attributes\n"); + goto port_cleanup; + } } ret = setup_port_pkey_list(device); @@ -624,7 +627,8 @@ int ib_register_device(struct ib_device *device, const char *name, list_for_each_entry(client, &client_list, list) if (!add_client_context(device, client) && client->add) - client->add(device); + if (device->kverbs_provider || client->no_kverbs_req) + client->add(device); down_write(&lists_rwsem); list_add_tail(&device->core_list, &device_list); @@ -721,7 +725,8 @@ int ib_register_client(struct ib_client *client) list_for_each_entry(device, &device_list, core_list) if (!add_client_context(device, client) && client->add) - client->add(device); + if (device->kverbs_provider || client->no_kverbs_req) + client->add(device); down_write(&lists_rwsem); list_add_tail(&client->list, &client_list); @@ -920,6 +925,9 @@ int ib_query_port(struct ib_device *device, union ib_gid gid; int err; + if (!device->ops.query_port) + return -EOPNOTSUPP; + if (!rdma_is_port_valid(device, port_num)) return -EINVAL; @@ -1043,6 +1051,9 @@ int ib_enum_all_devs(nldev_callback nldev_cb, struct sk_buff *skb, int ib_query_pkey(struct ib_device *device, u8 port_num, u16 index, u16 *pkey) { + if (!device->ops.query_pkey) + return -EOPNOTSUPP; + if (!rdma_is_port_valid(device, port_num)) return -EINVAL; diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index fb0007aa0c27..0eafee9a2ffc 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -1127,6 +1127,7 @@ static const struct file_operations uverbs_mmap_fops = { static struct ib_client uverbs_client = { .name = "uverbs", + .no_kverbs_req = true, .add = ib_uverbs_add_one, .remove = ib_uverbs_remove_one }; diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index c51e2505a9ad..54be4521c235 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -257,6 +257,10 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags, struct ib_pd *pd; int mr_access_flags = 0; + if (!device->ops.alloc_pd || + (mr_access_flags && !pd->device->ops.get_dma_mr)) + return ERR_PTR(-EOPNOTSUPP); + pd = device->ops.alloc_pd(device, NULL, NULL); if (IS_ERR(pd)) return pd; @@ -320,6 +324,10 @@ void ib_dealloc_pd(struct ib_pd *pd) { int ret; + if (!pd->device->ops.dealloc_pd || + (pd->__internal_mr && !pd->device->ops.dereg_mr)) + return; + if (pd->__internal_mr) { ret = pd->device->ops.dereg_mr(pd->__internal_mr); WARN_ON(ret); @@ -1124,10 +1132,12 @@ static struct ib_qp *ib_create_xrc_qp(struct ib_qp *qp, qp = __ib_open_qp(real_qp, qp_init_attr->event_handler, qp_init_attr->qp_context); - if (!IS_ERR(qp)) + if (!IS_ERR(qp)) { __ib_insert_xrcd_qp(qp_init_attr->xrcd, real_qp); - else - real_qp->device->ops.destroy_qp(real_qp); + } else { + if (real_qp->device->ops.destroy_qp) + real_qp->device->ops.destroy_qp(real_qp); + } return qp; } @@ -1604,6 +1614,9 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr, const struct ib_gid_attr *old_sgid_attr_alt_av; int ret; + if (!qp->device->ops.modify_qp) + return -EOPNOTSUPP; + if (attr_mask & IB_QP_AV) { ret = rdma_fill_sgid_attr(qp->device, &attr->ah_attr, &old_sgid_attr_av); @@ -1841,6 +1854,9 @@ int ib_destroy_qp(struct ib_qp *qp) struct ib_qp_security *sec; int ret; + if (!qp->device->ops.destroy_qp) + return -EOPNOTSUPP; + WARN_ON_ONCE(qp->mrs_used > 0); if (atomic_read(&qp->usecnt)) @@ -1928,6 +1944,9 @@ EXPORT_SYMBOL(rdma_set_cq_moderation); int ib_destroy_cq(struct ib_cq *cq) { + if (!cq->device->ops.destroy_cq) + return -EOPNOTSUPP; + if (atomic_read(&cq->usecnt)) return -EBUSY; @@ -1951,6 +1970,9 @@ int ib_dereg_mr(struct ib_mr *mr) struct ib_dm *dm = mr->dm; int ret; + if (!mr->device->ops.dereg_mr) + return -EOPNOTSUPP; + rdma_restrack_del(&mr->res); ret = mr->device->ops.dereg_mr(mr); if (!ret) { diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 94fe253d4956..072fd8be3355 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -149,6 +149,9 @@ static int get_port_state(struct ib_device *ibdev, struct ib_port_attr attr; int ret; + if (!ibdev->ops.query_port) + return -EOPNOTSUPP; + memset(&attr, 0, sizeof(attr)); ret = ibdev->ops.query_port(ibdev, port_num, &attr); if (!ret) diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index c073a4720d28..fb6074d2e394 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -2566,6 +2566,8 @@ struct ib_device { __be64 node_guid; u32 local_dma_lkey; u16 is_switch:1; + /* Indicates kernel verbs support, should not be used in drivers */ + u8 kverbs_provider:1; u8 node_type; u8 phys_port_cnt; struct ib_device_attr attrs; @@ -2620,6 +2622,9 @@ struct ib_client { const struct sockaddr *addr, void *client_data); struct list_head list; + + /* kverbs are not required by the client */ + u8 no_kverbs_req:1; }; struct ib_device *ib_alloc_device(size_t size);
Drivers that do not provide kernel verbs support should not be used by ib kernel clients and fail. In case a device does not implement all mandatory verbs for kverbs usage mark it as a non kverbs provider and prevent its usage for all clients except for uverbs. The device is marked as a non kverbs provider using the 'kverbs_provider' flag which should only be set by the core code. The clients can choose whether kverbs are requested for it usage using the 'no_kverbs_req' flag which is currently set for uverbs only. This patch allows drivers to remove mandatory verbs stubs and simply set the callback to NULL. The IB device will be registered as a non-kverbs provider. Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/core/device.c | 41 ++++++++++++++++++++++------------- drivers/infiniband/core/uverbs_main.c | 1 + drivers/infiniband/core/verbs.c | 28 +++++++++++++++++++++--- drivers/infiniband/hw/mlx5/main.c | 3 +++ include/rdma/ib_verbs.h | 5 +++++ 5 files changed, 60 insertions(+), 18 deletions(-)