| Message ID | 20180713102630.22193-2-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Jul 13, 2018 at 01:26:29PM +0300, Kamal Heib wrote: > Make sure the providers implement the verbs callbacks before calling > them, otherwise return -EOPNOTSUPP. > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > drivers/infiniband/core/mad.c | 10 ++++++---- > drivers/infiniband/core/uverbs_cmd.c | 3 ++- > drivers/infiniband/core/verbs.c | 6 ++++++ > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c > index 34e9b2768324..e528387605c8 100644 > +++ b/drivers/infiniband/core/mad.c > @@ -882,10 +882,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, > } > > /* No GRH for DR SMP */ > - ret = device->process_mad(device, 0, port_num, &mad_wc, NULL, > - (const struct ib_mad_hdr *)smp, mad_size, > - (struct ib_mad_hdr *)mad_priv->mad, > - &mad_size, &out_mad_pkey_index); > + ret = device->process_mad ? > + device->process_mad(device, 0, port_num, &mad_wc, NULL, > + (const struct ib_mad_hdr *)smp, mad_size, > + (struct ib_mad_hdr *)mad_priv->mad, > + &mad_size, &out_mad_pkey_index) : > + -EOPNOTSUPP; > switch (ret) > { > case IB_MAD_RESULT_SUCCESS | IB_MAD_RESULT_REPLY: > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index bd6eefaecbd6..6ab00f322b62 100644 > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -2507,7 +2507,8 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file, > goto out; > > resp.bad_wr = 0; > - ret = srq->device->post_srq_recv(srq, wr, &bad_wr); > + ret = srq->device->post_srq_recv ? > + srq->device->post_srq_recv(srq, wr, &bad_wr) : -EOPNOTSUPP; > uobj_put_obj_read(srq); > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index b6ceb6fd6a67..03b8c5448303 100644 > +++ b/drivers/infiniband/core/verbs.c > @@ -479,6 +479,9 @@ static struct ib_ah *_rdma_create_ah(struct ib_pd *pd, > { > struct ib_ah *ah; > > + if (!pd->device->create_ah) > + return ERR_PTR(-EOPNOTSUPP); > + > ah = pd->device->create_ah(pd, ah_attr, udata); > > if (!IS_ERR(ah)) { > @@ -911,6 +914,9 @@ int rdma_destroy_ah(struct ib_ah *ah) > struct ib_pd *pd; > int ret; > > + if (!ah->device->destroy_ah) > + return -EOPNOTSUPP; Instead of this, put a WARN_ON in ib_device_register like WARN_ON(ibdev->create_ah && !ibdev->destroy_ah) And don't test here.. We don't need to cope with broken drivers. 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 Fri, Jul 13, 2018 at 01:26:29PM +0300, Kamal Heib wrote: > Make sure the providers implement the verbs callbacks before calling > them, otherwise return -EOPNOTSUPP. > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > drivers/infiniband/core/mad.c | 10 ++++++---- > drivers/infiniband/core/uverbs_cmd.c | 3 ++- > drivers/infiniband/core/verbs.c | 6 ++++++ > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c > index 34e9b2768324..e528387605c8 100644 > +++ b/drivers/infiniband/core/mad.c > @@ -882,10 +882,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, > } > > /* No GRH for DR SMP */ > - ret = device->process_mad(device, 0, port_num, &mad_wc, NULL, > - (const struct ib_mad_hdr *)smp, mad_size, > - (struct ib_mad_hdr *)mad_priv->mad, > - &mad_size, &out_mad_pkey_index); > + ret = device->process_mad ? > + device->process_mad(device, 0, port_num, &mad_wc, NULL, > + (const struct ib_mad_hdr *)smp, mad_size, > + (struct ib_mad_hdr *)mad_priv->mad, > + &mad_size, &out_mad_pkey_index) : > + -EOPNOTSUPP; And this shouldn't be needed here, though I'm not entirely show how come.. iwarp devices should be blocked form using umad entirely. Maybe ib_create_send_mad() should fail if process_mad isn't defined? Not sure. Do all rocee and IB drivers define it? 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 Fri, Jul 13, 2018 at 10:18:10AM -0600, Jason Gunthorpe wrote: > On Fri, Jul 13, 2018 at 01:26:29PM +0300, Kamal Heib wrote: > > Make sure the providers implement the verbs callbacks before calling > > them, otherwise return -EOPNOTSUPP. > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > drivers/infiniband/core/mad.c | 10 ++++++---- > > drivers/infiniband/core/uverbs_cmd.c | 3 ++- > > drivers/infiniband/core/verbs.c | 6 ++++++ > > 3 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c > > index 34e9b2768324..e528387605c8 100644 > > +++ b/drivers/infiniband/core/mad.c > > @@ -882,10 +882,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, > > } > > > > /* No GRH for DR SMP */ > > - ret = device->process_mad(device, 0, port_num, &mad_wc, NULL, > > - (const struct ib_mad_hdr *)smp, mad_size, > > - (struct ib_mad_hdr *)mad_priv->mad, > > - &mad_size, &out_mad_pkey_index); > > + ret = device->process_mad ? > > + device->process_mad(device, 0, port_num, &mad_wc, NULL, > > + (const struct ib_mad_hdr *)smp, mad_size, > > + (struct ib_mad_hdr *)mad_priv->mad, > > + &mad_size, &out_mad_pkey_index) : > > + -EOPNOTSUPP; > > switch (ret) > > { > > case IB_MAD_RESULT_SUCCESS | IB_MAD_RESULT_REPLY: > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > > index bd6eefaecbd6..6ab00f322b62 100644 > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > @@ -2507,7 +2507,8 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file, > > goto out; > > > > resp.bad_wr = 0; > > - ret = srq->device->post_srq_recv(srq, wr, &bad_wr); > > + ret = srq->device->post_srq_recv ? > > + srq->device->post_srq_recv(srq, wr, &bad_wr) : -EOPNOTSUPP; > > uobj_put_obj_read(srq); > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > index b6ceb6fd6a67..03b8c5448303 100644 > > +++ b/drivers/infiniband/core/verbs.c > > @@ -479,6 +479,9 @@ static struct ib_ah *_rdma_create_ah(struct ib_pd *pd, > > { > > struct ib_ah *ah; > > > > + if (!pd->device->create_ah) > > + return ERR_PTR(-EOPNOTSUPP); > > > + > > ah = pd->device->create_ah(pd, ah_attr, udata); > > > > if (!IS_ERR(ah)) { > > @@ -911,6 +914,9 @@ int rdma_destroy_ah(struct ib_ah *ah) > > struct ib_pd *pd; > > int ret; > > > > + if (!ah->device->destroy_ah) > > + return -EOPNOTSUPP; > > Instead of this, put a WARN_ON in ib_device_register like > > WARN_ON(ibdev->create_ah && !ibdev->destroy_ah) > > And don't test here.. We don't need to cope with broken drivers. > > Jason I agree, the test shouldn't be in rdma_destroy_ah(), I'll remove it in V2. After doing more research I found that within ib_device_register() there is call for ib_device_check_mandatory() which will fail if {create, destroy}_ah() aren't implemented by the providers. I'll add another patch to this patch set that will remove this check and avoid ib_device_register() fail. With regards to adding the WARN_ON, I suggest to create a separate patch that will handle all the {create, destroy}_*() verbs. Thanks, Kamal -- 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 Fri, Jul 13, 2018 at 10:23:19AM -0600, Jason Gunthorpe wrote: > On Fri, Jul 13, 2018 at 01:26:29PM +0300, Kamal Heib wrote: > > Make sure the providers implement the verbs callbacks before calling > > them, otherwise return -EOPNOTSUPP. > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > drivers/infiniband/core/mad.c | 10 ++++++---- > > drivers/infiniband/core/uverbs_cmd.c | 3 ++- > > drivers/infiniband/core/verbs.c | 6 ++++++ > > 3 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c > > index 34e9b2768324..e528387605c8 100644 > > +++ b/drivers/infiniband/core/mad.c > > @@ -882,10 +882,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, > > } > > > > /* No GRH for DR SMP */ > > - ret = device->process_mad(device, 0, port_num, &mad_wc, NULL, > > - (const struct ib_mad_hdr *)smp, mad_size, > > - (struct ib_mad_hdr *)mad_priv->mad, > > - &mad_size, &out_mad_pkey_index); > > + ret = device->process_mad ? > > + device->process_mad(device, 0, port_num, &mad_wc, NULL, > > + (const struct ib_mad_hdr *)smp, mad_size, > > + (struct ib_mad_hdr *)mad_priv->mad, > > + &mad_size, &out_mad_pkey_index) : > > + -EOPNOTSUPP; > > And this shouldn't be needed here, though I'm not entirely show how > come.. iwarp devices should be blocked form using umad entirely. > > Maybe ib_create_send_mad() should fail if process_mad isn't defined? > Not sure. Do all rocee and IB drivers define it? > > Jason You are right, no need for this change, the test was already done within the following funcionts: smi_check_local_smp() and opa_smi_check_local_smp() smi_check_local_returning_smp() and opa_smi_check_local_returning_smp() I'll remove the test in V2. Thanks, Kamal -- 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/mad.c b/drivers/infiniband/core/mad.c index 34e9b2768324..e528387605c8 100644 --- a/drivers/infiniband/core/mad.c +++ b/drivers/infiniband/core/mad.c @@ -882,10 +882,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, } /* No GRH for DR SMP */ - ret = device->process_mad(device, 0, port_num, &mad_wc, NULL, - (const struct ib_mad_hdr *)smp, mad_size, - (struct ib_mad_hdr *)mad_priv->mad, - &mad_size, &out_mad_pkey_index); + ret = device->process_mad ? + device->process_mad(device, 0, port_num, &mad_wc, NULL, + (const struct ib_mad_hdr *)smp, mad_size, + (struct ib_mad_hdr *)mad_priv->mad, + &mad_size, &out_mad_pkey_index) : + -EOPNOTSUPP; switch (ret) { case IB_MAD_RESULT_SUCCESS | IB_MAD_RESULT_REPLY: diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index bd6eefaecbd6..6ab00f322b62 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -2507,7 +2507,8 @@ ssize_t ib_uverbs_post_srq_recv(struct ib_uverbs_file *file, goto out; resp.bad_wr = 0; - ret = srq->device->post_srq_recv(srq, wr, &bad_wr); + ret = srq->device->post_srq_recv ? + srq->device->post_srq_recv(srq, wr, &bad_wr) : -EOPNOTSUPP; uobj_put_obj_read(srq); diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index b6ceb6fd6a67..03b8c5448303 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -479,6 +479,9 @@ static struct ib_ah *_rdma_create_ah(struct ib_pd *pd, { struct ib_ah *ah; + if (!pd->device->create_ah) + return ERR_PTR(-EOPNOTSUPP); + ah = pd->device->create_ah(pd, ah_attr, udata); if (!IS_ERR(ah)) { @@ -911,6 +914,9 @@ int rdma_destroy_ah(struct ib_ah *ah) struct ib_pd *pd; int ret; + if (!ah->device->destroy_ah) + return -EOPNOTSUPP; + pd = ah->pd; ret = ah->device->destroy_ah(ah); if (!ret) {
Make sure the providers implement the verbs callbacks before calling them, otherwise return -EOPNOTSUPP. Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/core/mad.c | 10 ++++++---- drivers/infiniband/core/uverbs_cmd.c | 3 ++- drivers/infiniband/core/verbs.c | 6 ++++++ 3 files changed, 14 insertions(+), 5 deletions(-)