[ofw] [PATCH] core [4/5] - qp create sanity checks

[Reuven Amitai]

Hi,
 
The patch removes the #if 0 sanity check and add one check of
p_qp_create pointer.
 
I looked at IB spec and there isn't forbiddance of qp with sq depth of 0
(Although the low level driver will set it to 1).
Thanks for demonstrate one case of such use.
The sanity check is promoted from create_qp() (core\al_qp.c # 281) which
is called at the end of ib_create_qp.
Crash the app or assert the pointer is one way but IBAL performs sanity
checks all over the code and adhere this will be more consistent.
 
Reuven.
 
Index: core/al/al_pd.c

===================================================================

--- core/al/al_pd.c     (revision 1049)

+++ core/al/al_pd.c     (working copy)

@@ -333,24 +333,11 @@

            AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PD_HANDLE\n") );

            return IB_INVALID_PD_HANDLE;

      }

-

-#if 0

-     /* seems like no need in these checks */

-     if( !p_qp_create->rq_depth || !p_qp_create->sq_depth )

+     if( !p_qp_create || !ph_qp )

      {

-           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_WRS (%d:%d)\n",

-                 p_qp_create->rq_depth, p_qp_create->sq_depth ) );

-           return IB_INVALID_MAX_WRS;

+           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PARAMETER\n") );

+           return IB_INVALID_PARAMETER;

      }

-

-     if( !p_qp_create->rq_sge || !p_qp_create->sq_sge)

-     {

-           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_SGE (%d:%d)\n", 

-                 p_qp_create->rq_sge, p_qp_create->sq_sge ) );

-           return IB_INVALID_MAX_SGE;

-     }

-#endif     

-

      if (h_pd->obj.p_ci_ca && h_pd->obj.p_ci_ca->p_pnp_attr)

      {

            if ((p_qp_create->rq_depth >
h_pd->obj.p_ci_ca->p_pnp_attr->max_wrs) ||

 

 

 

________________________________

From: Sean Hefty [mailto:sean.hefty at intel.com] 
Sent: Thursday, April 10, 2008 9:20 PM
To: Reuven Amitai; ofw at lists.openfabrics.org
Subject: RE: [ofw] [PATCH] core [4/5]



Index: core/al/al_pd.c

===================================================================

--- core/al/al_pd.c    (revision 1047)

+++ core/al/al_pd.c    (working copy)

@@ -333,24 +333,22 @@

           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PD_HANDLE\n") );

           return IB_INVALID_PD_HANDLE;

     }

-

-#if 0

-    /* seems like no need in these checks */

-    if( !p_qp_create->rq_depth || !p_qp_create->sq_depth )

+    if( !p_qp_create || !ph_qp )

 

These sort of checks are pointless.  There's nothing that can be done at
run time.  Crash the app so that they get a backtrace and force the
programmer to fix them.

 

 

     {

-          AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_WRS (%d:%d)\n",

-                p_qp_create->rq_depth, p_qp_create->sq_depth ) );

-          return IB_INVALID_MAX_WRS;

+          AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PARAMETER\n") );

+          return IB_INVALID_PARAMETER;

     }

-

-    if( !p_qp_create->rq_sge || !p_qp_create->sq_sge)

+    if( !p_qp_create->rq_depth &&  !p_qp_create->sq_depth )

 

Couldn't a QP that's the target of RDMA reads/writes support depths of 0
on both?  The RDMA target information can be exchanged out of bounds or
during connection establishment.  (I know that Intel MPI exchanges RKey
information during connection setup.)

 

     {

-          AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_SGE (%d:%d)\n", 

-                p_qp_create->rq_sge, p_qp_create->sq_sge ) );

-          return IB_INVALID_MAX_SGE;

+          AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_SETTING\n") );

+          return IB_INVALID_SETTING;

     }

-#endif    

+    if( !p_qp_create->rq_sge && !p_qp_create->sq_sge)

+    {

 

These checks are related to checks against rq_depth and sq_depth,
respectively.

 

+          AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_SETTING\n") );

+          return IB_INVALID_SETTING;

 

+    }

     if (h_pd->obj.p_ci_ca && h_pd->obj.p_ci_ca->p_pnp_attr)

     {

           if ((p_qp_create->rq_depth >
h_pd->obj.p_ci_ca->p_pnp_attr->max_wrs) ||

 

 

- Sean

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080414/cdb5a2b5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qp_create_sanity_checks-fixed.patch
Type: application/octet-stream
Size: 1137 bytes
Desc: qp_create_sanity_checks-fixed.patch
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080414/cdb5a2b5/attachment.obj>