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

[Fab Tillier]

> From: Yossi Leybovich [mailto:sleybo at mellanox.co.il]
> Sent: Wednesday, September 14, 2005 7:23 AM
> 
> 3. bug in reg_svc - I think that the user_context was not set with the right
> value .
> I did this(core/al/al_reg_svc.c line 305):
> "
>         /* Copy the service registration information. */
> -       h_sa_reg->sa_req.user_context = h_sa_reg;
> +       h_sa_reg->sa_req.user_context = p_reg_svc_req->svc_context;
> "

This is already fixed.  Can you make sure you're using the latest source?

> I also notice that in IpoIB when you issue reg_svc request AL doesn't take
> reference to the adapter so I add this (ref and deref if fail )

This isn't needed - close_al won't return until all service registrations are
closed, so even if NDIS tries to disable the adapter while registrations are in
progress things will work properly.

That said, there's no harm in taking additional references, and if we change how
close_al behaves (no automatic cleanup), we'll need this.  I'll modify the patch
as noted below and apply this (but I will make it a disjoint patch and commit).

> Index: ulp/ipoib/kernel/ipoib_driver.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_driver.c     (revision 367)
> +++ ulp/ipoib/kernel/ipoib_driver.c     (working copy)
> @@ -2221,7 +2223,7 @@
>                 p_reg->p_adapter->hung = TRUE;
>                 p_reg->h_reg_svc = NULL;
>         }
> -
> +       cl_obj_deref(&p_reg->p_adapter->obj);
>         cl_obj_unlock( &p_reg->p_adapter->obj );

Note that technically, the release here should happen after the lock is released
since the code is still accessing the adapter object.

> 
>         IPOIB_EXIT( IPOIB_DBG_OID );
> Index: ulp/ipoib/kernel/ipoib_port.c
> ===================================================================

I think this has a lot of unnecessary changes.  For example, the rename from
get_mcast to get_bcast isn't really all that useful, and the function
__endpt_mgr_remove_bcast is never referenced.  I'll fix this and send a patch
for how I plan on handling SM change in IPoIB.

> --- ulp/ipoib/kernel/ipoib_port.c       (revision 367)
> +++ ulp/ipoib/kernel/ipoib_port.c       (working copy)
> @@ -421,7 +421,7 @@
>  *
> 
> ******************************************************************************
> /
>  static void
> -__port_get_mcast(
> +__port_get_bcast(
>         IN                              ipoib_port_t* const
> p_port );
> 
>  static void
> @@ -680,6 +680,7 @@
>  }
> 
> 
> +
>  static void
>  __port_free(
>         IN                              cl_obj_t* const
> p_obj )
> @@ -4045,6 +4046,30 @@
> 
> 
>  static ib_api_status_t
> +__endpt_mgr_remove_bcast(
> +       IN                              ipoib_port_t* const
> p_port)
> +{
> +       ipoib_endpt_t           *p_endpt;
> +
> +       IPOIB_ENTER( IPOIB_DBG_INIT );
> +
> +       /* Remove the broadcast endpoint. */
> +       p_endpt = __endpt_mgr_get_by_gid(p_port, &p_port-
> >ib_mgr.bcast_rec.mgid);
> +       if( !p_endpt )
> +       {
> +               IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR,
> +                       ("__endpt_mgr_get_by_gid failed.\n") );
> +               return IB_INVALID_GID;
> +       }
> +
> +       __endpt_mgr_remove( p_port, p_endpt );
> +
> +       IPOIB_EXIT( IPOIB_DBG_INIT );
> +       return IB_SUCCESS;
> +
> +}
> +
> +static ib_api_status_t
>  __endpt_mgr_add_bcast(
>         IN                              ipoib_port_t* const
> p_port,
>         IN                              ib_mcast_rec_t
> *p_mcast_rec )
> @@ -4353,7 +4378,7 @@
>                                 p_port_rec->port_info.link_width_active) );
>                         ipoib_set_rate( p_port->p_adapter,
>                                 p_port_rec->port_info.link_width_active );
> -                       __port_get_mcast( p_port );
> +                       __port_get_bcast( p_port );
>                 }
>                 else
>                 {
> @@ -4386,7 +4411,7 @@
> 
> 
>  static void
> -__port_get_mcast(
> +__port_get_bcast(
>         IN                              ipoib_port_t* const
> p_port )
>  {
>         ib_api_status_t         status;
> @@ -4716,7 +4741,7 @@
>                         state isn't IB_PNP_PORT_ADD or PORT_DOWN? */
>                         CL_ASSERT( p_port->p_adapter->state == IB_PNP_PORT_ADD
> ||
>                                 p_port->p_adapter->state ==
> IB_PNP_PORT_DOWN );
> -                       __port_get_mcast( p_port );
> +                       __port_get_bcast( p_port );
>                 }
>                 else
>                 {
> @@ -5034,3 +5059,19 @@
> 
>         IPOIB_EXIT( IPOIB_DBG_MCAST );
>  }
> +
> +
> +ib_api_status_t
> +ipoib_port_sm_change(
> +       IN                              ipoib_port_t* const
> p_port)
> +{
> +       ib_api_status_t         status = IB_SUCCESS;
> +
> +       IPOIB_ENTER( IPOIB_DBG_INIT );
> +
> +       p_port->p_adapter->hung = TRUE;
> +
> +       IPOIB_EXIT( IPOIB_DBG_INIT );
> +       return status;
> +
> +}
> Index: ulp/ipoib/kernel/ipoib_port.h
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.h       (revision 367)
> +++ ulp/ipoib/kernel/ipoib_port.h       (working copy)
> @@ -560,6 +560,10 @@
>         IN                              ipoib_port_t* const
> p_port,
>         IN              const   mac_addr_t
> mac );
> 
> +ib_api_status_t
> +ipoib_port_sm_change(
> +       IN                              ipoib_port_t* const
> p_port);
> +
>  void
>  ipoib_port_remove_endpt(
>         IN                              ipoib_port_t* const
> p_port,
>