[Openib-windows] RE: [PATCH] reregister\cl_vecto\reg_svc

[Fab Tillier]

Comments related to the changes to IBAL for supporting client reregister,
excluding all other changes...  Note that these are just comments - I will take
care of fixing things up, so no need to resend the patch.

I'll be committing this shortly (just the parts in the access layer to enable
reregister support).  I'll follow that with a commit to the HCA driver to
generate these events.

Thanks,

- Fab

> From: Yossi Leybovich [mailto:sleybo at mellanox.co.il]
> Sent: Wednesday, September 14, 2005 7:23 AM
> 
> Singed-off-by: Yossi Leybovich (sleybo at mellanox.co.il)
> Index: core/al/al_ci_ca_shared.c
> ===================================================================
> --- core/al/al_ci_ca_shared.c   (revision 367)
> +++ core/al/al_ci_ca_shared.c   (working copy)
> @@ -371,6 +371,7 @@
>  {
>         ib_ca_handle_t                  h_ca;
> 
> +       AL_ENTER(AL_DBG_CA);
>         CL_ASSERT( p_event_rec );
>         h_ca = p_event_rec->handle.h_ca;
> 
> @@ -379,6 +380,8 @@
> 
>         if( h_ca->pfn_event_cb )
>                 h_ca->pfn_event_cb( p_event_rec );
> +
> +       AL_EXIT(AL_DBG_CA);
>  }

This function is invoked for every client - I don't think we need the extra
debug output as it just ends up as extra noise in the debugger log.

It is also missing the case handler for IB_AE_CLIENT_REREGISTER in
ci_ca_process_event_cb.  I will add this.

> Index: core/al/kernel/al_pnp.c
> ===================================================================
> --- core/al/kernel/al_pnp.c     (revision 367)
> +++ core/al/kernel/al_pnp.c     (working copy)
> @@ -1465,7 +1465,7 @@
>         ib_ca_attr_t                    *p_old_ca_attr;
>         ib_api_status_t                 status;
> 
> -       CL_ENTER( AL_DBG_PNP, g_al_dbg_lvl );
> +       AL_ENTER( AL_DBG_PNP);
> 
>         UNUSED_PARAM( p_item );
>         CL_ASSERT( gp_pnp );
> @@ -1510,7 +1510,7 @@
>         deref_al_obj( &gp_pnp->obj );
>         gp_pnp->async_item_is_busy = FALSE;
> 
> -       CL_EXIT( AL_DBG_PNP, g_al_dbg_lvl );
> +       AL_EXIT( AL_DBG_PNP);
>  }

I'm going to hold off on this - in general, I only change these macros if I'm
changing the function.  If we want to do a large scale cleanup, we should do so
but treat all the files (per component) as a single patch.

> Index: core/al/kernel/al_smi.c
> ===================================================================
> --- core/al/kernel/al_smi.c     (revision 367)
> +++ core/al/kernel/al_smi.c     (working copy)
> @@ -1812,6 +1812,13 @@
>                         {
>                                 p_spl_qp_svc->base_lid = p_port_info-
> >base_lid;
>                                 p_spl_qp_svc->lmc =
> ib_port_info_get_lmc( p_port_info );
> +                               if (p_port_info->subnet_timeout & 0x80)
> +                               {
> +                                       AL_TRACE(AL_DBG_PNP,(" setting the
> sm_lid to 0 client reregister event \n"));
> +                                       ci_ca_lock_attr(p_spl_qp_svc-
> >obj.p_ci_ca);
> +                                       p_spl_qp_svc->obj.p_ci_ca->p_pnp_attr-
> >p_port_attr->sm_lid= 0;
> +                                       ci_ca_unlock_attr(p_spl_qp_svc-
> >obj.p_ci_ca);
> +                               }
>                         }
>                 }
>         }

I fixed this to keep the lines <= 80 characters.  Other than that, the change
looks good.

> Index: inc/iba/ib_types.h
> ===================================================================
> --- inc/iba/ib_types.h  (revision 367)
> +++ inc/iba/ib_types.h  (working copy)
> @@ -3839,38 +3839,38 @@
>  #define IB_PORT_LINK_SPEED_SUPPORTED_MASK                      0xF0
> 
> 
> -#define IB_PORT_CAP_RESV1                      (CL_NTOH32(0x00000001))
> -#define IB_PORT_CAP_IS_SM                      (CL_NTOH32(0x00000002))
> -#define IB_PORT_CAP_HAS_NOTICE         (CL_NTOH32(0x00000004))
> -#define IB_PORT_CAP_HAS_TRAP           (CL_NTOH32(0x00000008))

<snip>

> +#define IB_PORT_CAP_RESV0         (CL_NTOH32(0x00000001))
> +#define IB_PORT_CAP_IS_SM         (CL_NTOH32(0x00000002))
> +#define IB_PORT_CAP_HAS_NOTICE    (CL_NTOH32(0x00000004))
> +#define IB_PORT_CAP_HAS_TRAP      (CL_NTOH32(0x00000008))

<snip>

Again, don't convert tabs to spaces.  The majority of the code base is setup
with a tab width of 4.  I'll take care of it, so no need to resend the patch.

> @@ -7521,7 +7529,7 @@
>          * timeout = 4.096 microseconds * 2^subnet_timeout
>          */
>         uint8_t                                 subnet_timeout;
> -
> +       uint8_t                                 client_reregistration;

This isn't used anymore so I eliminated it.