[openib-general] Re: [PATCH] [kdapl CM] Fix endian conversions of service ID

[James Lentini]

I committed the fix for this in revision 2486. I made one small 
simplification: I used the dapl_cm_id's param.service_id field 
throughout.

On Wed, 25 May 2005, Hal Rosenstock wrote:

> On Wed, 2005-05-25 at 11:38, James Lentini wrote:
>> halr> On Tue, 2005-05-24 at 15:20, James Lentini wrote:
>> halr> > halr> [kdapl CM] Fix endian conversions of service ID
>> halr> > halr> Problem pointed out by James Lentini
>> halr> > halr>
>> halr> > halr> Signed-off-by: Hal Rosenstock <halr at voltaire.com>
>> halr> > halr>
>> halr> > halr> Index: dapl_openib_cm.c
>> halr> > halr> ===================================================================
>> halr> > halr> -- dapl_openib_cm.c    (revision 2468)
>> halr> > halr> +++ dapl_openib_cm.c    (working copy)
>> halr> > halr> @@ -309,7 +309,7 @@
>> halr> > halr>         if (conn->dapl_path.mtu > IB_MTU_1024)
>> halr> > halr>                 conn->dapl_path.mtu = IB_MTU_1024;
>> halr> > halr>
>> halr> > halr> -       conn->param.service_id = be64_to_cpu(conn->service_id);
>> halr> > halr> +       conn->param.service_id = conn->service_id;
>> halr> >
>> halr> > With the change to dapl_ib_connect below, the conn->service_id is in
>> halr> > CPU byte order at this point. The conn->param is a ib_cm_req_param
>> halr> > structure. The comment describing this structure's service_id field
>> halr> > says that it should be in network (big endian) byte order.
>> halr> >
>> halr> > So...
>> halr> >
>> halr> > halr>         conn->param.primary_path = &conn->dapl_path;
>> halr> > halr>         conn->param.alternate_path = NULL;
>> halr> > halr>
>> halr> > halr> @@ -445,8 +445,7 @@
>> halr> > halr>         conn->param.local_cm_response_timeout =
>> halr> > halr> DAPL_OPENIB_CM_RESPONSE_TIMEOUT;
>> halr> > halr>         conn->param.max_cm_retries = DAPL_OPENIB_MAX_CM_RETRIES;
>> halr> > halr>
>> halr> > halr> -       memcpy(&conn->service_id, &remote_conn_qual, sizeof
>> halr> > halr> conn->service_id);
>> halr> > halr> -
>> halr> > halr> +       conn->service_id = be64_to_cpu(remote_conn_qual);
>> halr> >
>> halr> > ...that makes me think we should change the line above to
>> halr> >
>> halr> > 	      conn->service_id = remote_conn_qual;
>> halr> >
>> halr> > and require that consumer's specify their connection qualifier values
>> halr> > in network byte order here and ...
>> halr>
>> halr> I think the convention OpenIB has been using is to supply parameters in
>> halr> CPU endian but it can work either way.
>>
>> The comments in ib_cm.h say that service id parameters should be in
>> network byte order. Are these incorrect?
>
> No. I was referring to your alternative of where this is performed.
>
>> halr>
>> halr> > halr>         conn->remote_ia_address = remote_ia_address;
>> halr> > halr>         conn->dapl_comp.fn = &dapl_rt_comp_handler;
>> halr> > halr>         conn->dapl_comp.context = conn;
>> halr> > halr> @@ -627,7 +626,7 @@
>> halr> > halr>         }
>> halr> > halr>
>> halr> > halr>         status = ib_cm_listen(sp_ptr->cm_srvc_handle,
>> halr> > halr> -                             be64_to_cpu(sp_ptr->conn_qual), 0);
>> halr> > halr> +                             cpu_to_be64(sp_ptr->conn_qual), 0);
>> halr> >
>> halr> > ... do the same here. What do you think?
>> halr>
>> halr>
>