[ofw] RE: Bad merge in checking 1440

[Sean Hefty]

To help, I included this comment when I sent out the patch for review:


As far as I could tell, there is no code that ever results in passing the work
request structure between userspace and the kernel.  We can have a cleaner,
more efficient interface if we do not try to maintain a single structure that
is the same for a 32-bit userspace application and a 64-bit kernel, especially
if the wr structure is not used in that context in practice.

In fact it appears that there's a lot of kernel 'proxy' code that is unreachable
given the existing libraries and drivers that we should consider removing.


... so, basically, I modified the WR structure based on its actual use as far as
I could tell.  If we need to support userspace applications posting WRs without
a userspace library, then I would much rather take the performance hit
converting the wr structure when dropping into the kernel, versus gunking up the
user interface.

>Your changes in revision 1440 reverted some of the handle exchange\validation
>between UAL and KAL that shouldn't have been.

Given what I mentioned above, do you see any actual issues with the code? 

>Index: core/al/kernel/al_proxy_verbs.c
>===================================================================
>--- core/al/kernel/al_proxy_verbs.c     (revision 1430)
>+++ core/al/kernel/al_proxy_verbs.c     (working copy)
>@@ -2356,7 +2356,7 @@
>                {
>                        /* Validate the AV handle for UD */
>                        h_av = (ib_av_handle_t)al_hdl_ref( p_context->h_al,
>-                               p_wr[i].dgrm.ud.h_av_padding, AL_OBJ_TYPE_H_AV
>);
>+                               (ULONG_PTR) p_wr[i].dgrm.ud.h_av,
>AL_OBJ_TYPE_H_AV );
>                        if( !h_av )
>                        {
>                                status = IB_INVALID_AV_HANDLE;Index:
>core/al/user/ual_qp.c
>===================================================================
>--- core/al/user/ual_qp.c       (revision 1430)
>+++ core/al/user/ual_qp.c       (working copy)
>@@ -119,8 +119,8 @@
>                p_qp_ioctl->in.send_wr[num_wr] = *p_wr;
>                if( h_qp->type == IB_QPT_UNRELIABLE_DGRM )
>                {
>-                       p_qp_ioctl->in.send_wr[num_wr].dgrm.ud.h_av_padding =
>-                               p_wr->dgrm.ud.h_av->obj.hdl;
>+                       p_qp_ioctl->in.send_wr[num_wr].dgrm.ud.h_av =
>+                               (ib_av_handle_t) (ULONG_PTR) p_wr-
>>dgrm.ud.h_av->obj.hdl;
>                }
>                num_wr++;
>                cl_memcpy(
>
>You also lost the TO_LONG_PTR use in the ib_send_wr_t structure, which is
>exchanged between UAL and KAL.  If you remove the code that exchanges the WR
>between user and kernel mode you can eliminate the TO_LONG_PTR use from that
>structure.  The WR is exchanged in an IOCTL if a UVP doesn't kernel bypass for
>QP operations.  Alternatively, you can create an IOCTL-safe WR structure and
>change the IOCTL handling to copy the WR structure field by field into the
>IOCTL structure, to keep the functionality but eliminate the TO_LONG_PTR use in
>the public structure.

- Sean