diff mbox series

[rdma-next,v3,1/3] RDMA/core: Remove mandatory verbs check

Message ID 20180725201045.26995-2-kamalheib1@gmail.com (mailing list archive)
State Superseded
Headers show
Series RDMA: handle pointless functions | expand

Commit Message

Kamal Heib July 25, 2018, 8:10 p.m. UTC 
Removing the check for mandatory verbs because not all providers are
implementing them.

Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/core/device.c | 45 ----------------------------------------
 1 file changed, 45 deletions(-)

Comments

Leon Romanovsky July 26, 2018, 5:47 a.m. UTC | #1 
On Wed, Jul 25, 2018 at 11:10:43PM +0300, Kamal Heib wrote:
> Removing the check for mandatory verbs because not all providers are
> implementing them.
>
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> ---
>  drivers/infiniband/core/device.c | 45 ----------------------------------------
>  1 file changed, 45 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index b8144f194777..c6fd0ec344b7 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -93,46 +93,6 @@ static struct notifier_block ibdev_lsm_nb = {
>  	.notifier_call = ib_security_change,
>  };
>
> -static int ib_device_check_mandatory(struct ib_device *device)
> -{
> -#define IB_MANDATORY_FUNC(x) { offsetof(struct ib_device, x), #x }
> -	static const struct {
> -		size_t offset;
> -		char  *name;
> -	} mandatory_table[] = {
> -		IB_MANDATORY_FUNC(query_device),
> -		IB_MANDATORY_FUNC(query_port),
> -		IB_MANDATORY_FUNC(query_pkey),
> -		IB_MANDATORY_FUNC(alloc_pd),
> -		IB_MANDATORY_FUNC(dealloc_pd),
> -		IB_MANDATORY_FUNC(create_ah),
> -		IB_MANDATORY_FUNC(destroy_ah),
> -		IB_MANDATORY_FUNC(create_qp),
> -		IB_MANDATORY_FUNC(modify_qp),
> -		IB_MANDATORY_FUNC(destroy_qp),
> -		IB_MANDATORY_FUNC(post_send),
> -		IB_MANDATORY_FUNC(post_recv),
> -		IB_MANDATORY_FUNC(create_cq),
> -		IB_MANDATORY_FUNC(destroy_cq),
> -		IB_MANDATORY_FUNC(poll_cq),
> -		IB_MANDATORY_FUNC(req_notify_cq),
> -		IB_MANDATORY_FUNC(get_dma_mr),
> -		IB_MANDATORY_FUNC(dereg_mr),
> -		IB_MANDATORY_FUNC(get_port_immutable)
> -	};
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
> -		if (!*(void **) ((void *) device + mandatory_table[i].offset)) {
> -			pr_warn("Device %s is missing mandatory function %s\n",
> -				device->name, mandatory_table[i].name);
> -			return -EINVAL;
> -		}
> -	}

It is not enough to remove this check, you need to add checks if
function exists in all places with calls to those mandatory functions.

It is better have some general function/macro for that, something like that:

#define is_cmd_suported(dev, name) (dev->name)

In long run, we will do in kernel something similar to verbs_context_ops in rdma-core.

Thanks


> -
> -	return 0;
> -}
> -
>  static struct ib_device *__ib_device_get_by_index(u32 index)
>  {
>  	struct ib_device *device;
> @@ -502,11 +462,6 @@ int ib_register_device(struct ib_device *device,
>  			goto out;
>  	}
>
> -	if (ib_device_check_mandatory(device)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	ret = read_port_immutable(device);
>  	if (ret) {
>  		pr_warn("Couldn't create per port immutable data %s\n",
> --
> 2.14.4
>
--
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
Jason Gunthorpe July 26, 2018, 2:45 p.m. UTC | #2 
On Thu, Jul 26, 2018 at 08:47:13AM +0300, Leon Romanovsky wrote:
> On Wed, Jul 25, 2018 at 11:10:43PM +0300, Kamal Heib wrote:
> > Removing the check for mandatory verbs because not all providers are
> > implementing them.
> >
> > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> >  drivers/infiniband/core/device.c | 45 ----------------------------------------
> >  1 file changed, 45 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index b8144f194777..c6fd0ec344b7 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -93,46 +93,6 @@ static struct notifier_block ibdev_lsm_nb = {
> >  	.notifier_call = ib_security_change,
> >  };
> >
> > -static int ib_device_check_mandatory(struct ib_device *device)
> > -{
> > -#define IB_MANDATORY_FUNC(x) { offsetof(struct ib_device, x), #x }
> > -	static const struct {
> > -		size_t offset;
> > -		char  *name;
> > -	} mandatory_table[] = {
> > -		IB_MANDATORY_FUNC(query_device),
> > -		IB_MANDATORY_FUNC(query_port),
> > -		IB_MANDATORY_FUNC(query_pkey),
> > -		IB_MANDATORY_FUNC(alloc_pd),
> > -		IB_MANDATORY_FUNC(dealloc_pd),
> > -		IB_MANDATORY_FUNC(create_ah),
> > -		IB_MANDATORY_FUNC(destroy_ah),
> > -		IB_MANDATORY_FUNC(create_qp),
> > -		IB_MANDATORY_FUNC(modify_qp),
> > -		IB_MANDATORY_FUNC(destroy_qp),
> > -		IB_MANDATORY_FUNC(post_send),
> > -		IB_MANDATORY_FUNC(post_recv),
> > -		IB_MANDATORY_FUNC(create_cq),
> > -		IB_MANDATORY_FUNC(destroy_cq),
> > -		IB_MANDATORY_FUNC(poll_cq),
> > -		IB_MANDATORY_FUNC(req_notify_cq),
> > -		IB_MANDATORY_FUNC(get_dma_mr),
> > -		IB_MANDATORY_FUNC(dereg_mr),
> > -		IB_MANDATORY_FUNC(get_port_immutable)
> > -	};
> > -	int i;
> > -
> > -	for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
> > -		if (!*(void **) ((void *) device + mandatory_table[i].offset)) {
> > -			pr_warn("Device %s is missing mandatory function %s\n",
> > -				device->name, mandatory_table[i].name);
> > -			return -EINVAL;
> > -		}
> > -	}
> 
> It is not enough to remove this check, you need to add checks if
> function exists in all places with calls to those mandatory functions.

Ugh, Okay, that is something I have some partial plans for down the
road.

> It is better have some general function/macro for that, something
> like that:
> 
> #define is_cmd_suported(dev, name) (dev->name)
> 
> In long run, we will do in kernel something similar to
> verbs_context_ops in rdma-core.

So, what we should do is drop off all functions here that have
internal protections in verbs/etc. Since Kamal's patches add these
protections to the _ah family lets just do what he originally
proposed..

Thus ib_device_check_mandatory looks for functions that the kernel
internally assumes are not NULL, and has nothing to do with IBTA,
specs, etc.

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
Kamal Heib July 27, 2018, 6:20 p.m. UTC | #3 
On Thu, Jul 26, 2018 at 08:45:21AM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 26, 2018 at 08:47:13AM +0300, Leon Romanovsky wrote:
> > On Wed, Jul 25, 2018 at 11:10:43PM +0300, Kamal Heib wrote:
> > > Removing the check for mandatory verbs because not all providers are
> > > implementing them.
> > >
> > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > >  drivers/infiniband/core/device.c | 45 ----------------------------------------
> > >  1 file changed, 45 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index b8144f194777..c6fd0ec344b7 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -93,46 +93,6 @@ static struct notifier_block ibdev_lsm_nb = {
> > >  	.notifier_call = ib_security_change,
> > >  };
> > >
> > > -static int ib_device_check_mandatory(struct ib_device *device)
> > > -{
> > > -#define IB_MANDATORY_FUNC(x) { offsetof(struct ib_device, x), #x }
> > > -	static const struct {
> > > -		size_t offset;
> > > -		char  *name;
> > > -	} mandatory_table[] = {
> > > -		IB_MANDATORY_FUNC(query_device),
> > > -		IB_MANDATORY_FUNC(query_port),
> > > -		IB_MANDATORY_FUNC(query_pkey),
> > > -		IB_MANDATORY_FUNC(alloc_pd),
> > > -		IB_MANDATORY_FUNC(dealloc_pd),
> > > -		IB_MANDATORY_FUNC(create_ah),
> > > -		IB_MANDATORY_FUNC(destroy_ah),
> > > -		IB_MANDATORY_FUNC(create_qp),
> > > -		IB_MANDATORY_FUNC(modify_qp),
> > > -		IB_MANDATORY_FUNC(destroy_qp),
> > > -		IB_MANDATORY_FUNC(post_send),
> > > -		IB_MANDATORY_FUNC(post_recv),
> > > -		IB_MANDATORY_FUNC(create_cq),
> > > -		IB_MANDATORY_FUNC(destroy_cq),
> > > -		IB_MANDATORY_FUNC(poll_cq),
> > > -		IB_MANDATORY_FUNC(req_notify_cq),
> > > -		IB_MANDATORY_FUNC(get_dma_mr),
> > > -		IB_MANDATORY_FUNC(dereg_mr),
> > > -		IB_MANDATORY_FUNC(get_port_immutable)
> > > -	};
> > > -	int i;
> > > -
> > > -	for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
> > > -		if (!*(void **) ((void *) device + mandatory_table[i].offset)) {
> > > -			pr_warn("Device %s is missing mandatory function %s\n",
> > > -				device->name, mandatory_table[i].name);
> > > -			return -EINVAL;
> > > -		}
> > > -	}
> > 
> > It is not enough to remove this check, you need to add checks if
> > function exists in all places with calls to those mandatory functions.
> 
> Ugh, Okay, that is something I have some partial plans for down the
> road.
> 
> > It is better have some general function/macro for that, something
> > like that:
> > 
> > #define is_cmd_suported(dev, name) (dev->name)
> > 
> > In long run, we will do in kernel something similar to
> > verbs_context_ops in rdma-core.
> 
> So, what we should do is drop off all functions here that have
> internal protections in verbs/etc. Since Kamal's patches add these
> protections to the _ah family lets just do what he originally
> proposed..
> 
> Thus ib_device_check_mandatory looks for functions that the kernel
> internally assumes are not NULL, and has nothing to do with IBTA,
> specs, etc.
> 
> Jason

OK, I'll resend the patch that remove {create, destroy}_ah from
ib_device_check_mandatory(). 

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 mbox series

Patch

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index b8144f194777..c6fd0ec344b7 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -93,46 +93,6 @@  static struct notifier_block ibdev_lsm_nb = {
 	.notifier_call = ib_security_change,
 };
 
-static int ib_device_check_mandatory(struct ib_device *device)
-{
-#define IB_MANDATORY_FUNC(x) { offsetof(struct ib_device, x), #x }
-	static const struct {
-		size_t offset;
-		char  *name;
-	} mandatory_table[] = {
-		IB_MANDATORY_FUNC(query_device),
-		IB_MANDATORY_FUNC(query_port),
-		IB_MANDATORY_FUNC(query_pkey),
-		IB_MANDATORY_FUNC(alloc_pd),
-		IB_MANDATORY_FUNC(dealloc_pd),
-		IB_MANDATORY_FUNC(create_ah),
-		IB_MANDATORY_FUNC(destroy_ah),
-		IB_MANDATORY_FUNC(create_qp),
-		IB_MANDATORY_FUNC(modify_qp),
-		IB_MANDATORY_FUNC(destroy_qp),
-		IB_MANDATORY_FUNC(post_send),
-		IB_MANDATORY_FUNC(post_recv),
-		IB_MANDATORY_FUNC(create_cq),
-		IB_MANDATORY_FUNC(destroy_cq),
-		IB_MANDATORY_FUNC(poll_cq),
-		IB_MANDATORY_FUNC(req_notify_cq),
-		IB_MANDATORY_FUNC(get_dma_mr),
-		IB_MANDATORY_FUNC(dereg_mr),
-		IB_MANDATORY_FUNC(get_port_immutable)
-	};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
-		if (!*(void **) ((void *) device + mandatory_table[i].offset)) {
-			pr_warn("Device %s is missing mandatory function %s\n",
-				device->name, mandatory_table[i].name);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
 static struct ib_device *__ib_device_get_by_index(u32 index)
 {
 	struct ib_device *device;
@@ -502,11 +462,6 @@  int ib_register_device(struct ib_device *device,
 			goto out;
 	}
 
-	if (ib_device_check_mandatory(device)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	ret = read_port_immutable(device);
 	if (ret) {
 		pr_warn("Couldn't create per port immutable data %s\n",