[ofw] changes to ib_wc committed to svn

[Fab Tillier]

> From: Sean Hefty
> Sent: Wednesday, July 23, 2008 1:51 PM
>
> I must have missed seeing the patch to review the following change to
> ib_wc:

Me too...

Besides agreeing with Sean about the vendor_specific field, I have issues with the changes to IPoIB.  I'll put the changes here, as if they had been sent out for review, and comment inline.

>Index: ulp/ipoib/kernel/ipoib_port.c
>===================================================================
>--- ulp/ipoib/kernel/ipoib_port.c      (revision 1429)
>+++ ulp/ipoib/kernel/ipoib_port.c      (revision 1430)
>@@ -1954,6 +1954,7 @@
>
>               }
>               /* Successful completion.  Get the receive information. */
>+              p_desc->ndis_csum.Value = (ULONG) p_wc->csum_ok;

If the csum_ok field is vendor specific (last I checked the IB spec didn't define a standard method for reporting IP checksum capabilities) then this assignment needs to check that the underlying HCA hardware supports checksum offload.

>               cl_perf_start( GetRecvEndpts );
>               __recv_get_endpts( p_port, p_desc, p_wc, &p_src, &p_dst );
>               cl_perf_stop( &p_port->p_adapter->perf, GetRecvEndpts );
>@@ -2501,7 +2502,6 @@
>       NDIS_STATUS                                                     status;
>       uint32_t                                                        pkt_filter;
>       ip_stat_sel_t                                           type;
>-      NDIS_TCP_IP_CHECKSUM_PACKET_INFO        chksum;
>       PERF_DECLARE( GetNdisPkt );
>
>       IPOIB_ENTER( IPOIB_DBG_RECV );
>@@ -2582,14 +2582,9 @@
>               return IB_INSUFFICIENT_RESOURCES;
>       }
>
>-      /* Flag the checksums as having been calculated. */
>-      chksum.Value = 0;
>-      chksum.Receive.NdisPacketTcpChecksumSucceeded = TRUE;
>-      chksum.Receive.NdisPacketUdpChecksumSucceeded = TRUE;
>-      chksum.Receive.NdisPacketIpChecksumSucceeded = TRUE;
>-      NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet, TcpIpChecksumPacketInfo ) =
>-              (void*)(uintn_t)chksum.Value;
>-
>+      /* Get the checksums directly from packet information. */
>+      NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet, TcpIpChecksumPacketInfo ) =
>+              (PVOID) (uintn_t) (p_desc->ndis_csum.Value);

Ok, so here, you assume that p_desc->ndis_csum.Value is properly formatted. That means that the assignment above (p_desc->ndis_csum.Value = (ULONG) p_wc->csum_ok) does the right thing.  However, the MTHCA driver doesn't clear this field.

You've also lost the ability to eliminate receive checksum validation.  IB has 32-bit CRC which would cause any corrupted packets to get dropped. If you assume that packets are being put on the wire correctly, then if they are received at all they haven't been corrupted.

>       ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len );
>
>       IPOIB_EXIT( IPOIB_DBG_RECV );
>@@ -3618,6 +3613,8 @@
> {
>       NDIS_STATUS                     status;
>       int32_t                         hdr_idx;
>+      PNDIS_PACKET_EXTENSION                          PktExt;
>+      PNDIS_TCP_IP_CHECKSUM_PACKET_INFO       pChecksumPktInfo; //NDIS 5.1
>
>       PERF_DECLARE( SendMgrFilter );
>
>@@ -3652,6 +3649,23 @@
>       p_desc->wr.wr_id = (uintn_t)p_desc->p_pkt;
>       p_desc->wr.wr_type = WR_SEND;
>       p_desc->wr.send_opt = IB_SEND_OPT_SIGNALED;
>+
>+      PktExt = NDIS_PACKET_EXTENSION_FROM_PACKET(p_desc->p_pkt);
>+      pChecksumPktInfo = (PNDIS_TCP_IP_CHECKSUM_PACKET_INFO)&PktExt->NdisPacketInfo[TcpIpChecksumPacketInfo];
>+      if(p_port->p_adapter->params.send_chksum_offload &
>+              (pChecksumPktInfo->Transmit.NdisPacketChecksumV4 || pChecksumPktInfo->Transmit.NdisPacketChecksumV6))
>+      {
>+              // Set transimition checksum offloading
>+              if (pChecksumPktInfo->Transmit.NdisPacketIpChecksum)
>+              {
>+                      p_desc->wr.send_opt |= IB_SEND_OPT_TX_IP_CSUM;
>+              }
>+              if(pChecksumPktInfo->Transmit.NdisPacketTcpChecksum  )
>+              {
>+                      p_desc->wr.send_opt |= IB_SEND_OPT_TX_TCP_UDP_CSUM;
>+              }

What happens if these options are set and the underlying HCA doesn't support checksum offload?  Does the WR just get sent or does it fail?

All in all, I think using the vendor specific field for checksum notifications seems lacking proper design, and more importantly, community input. A few issues from a quick glance:
- it should be clear that it matches the NDIS_TCP_IP_CHECKSUM_PACKET_INFO definition.
- either checksum offload is vendor specific or it isn't.  If it is, then:
        1. the IB_SEND_OPT_TX_XXX_CSUM values should be in the
        IB_SEND_OPT_VEND_MASK range
        2. The ib_wc_t structure stays unchanged
        3. IPoIB checks which HCA it runs on and has HCA-specific code to
        handle checksum offload.
        4. ib_ca_attr_t is unchanged
If it isn't then the vendor_specific field should get overloaded with non-vendor specific information and csum_ok should be put into the recv.ud part of the ib_wc_t structure.
- MTHCA needs to be updated so that it properly clears csum_ok.

-Fab