[ofw] [mthca/mlx4] ChkSum bitfields mismatch?

[Fab Tillier]

Hi Xalex,

>
> You are right. This caused to IP checksum to be recalculated again in
> SW
> stack, in spite of the HW offload.
> Bellow is the patch that fixes this problem and persuade IPoIB to
> recalculate csum even if HW returns good value in a case of
> CSUM_DISABLED.
> Tzachi, please apply it
>
> Index: core/al/al_av.c
> ===================================================================
> --- core/al/al_av.c     (revision 3107)
> +++ core/al/al_av.c     (working copy)
> @@ -153,12 +153,12 @@
>                 return IB_INVALID_PD_HANDLE;
>         }
>
> -       status = __check_av_port(h_pd->obj.p_ci_ca, p_av_attr);
> +       /*status = __check_av_port(h_pd->obj.p_ci_ca, p_av_attr);
>         if( status != IB_SUCCESS )
>         {
>                 AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
> ("IB_INVALID_PORT\n") );
>                 return status;
> -       }
> +       }*/

I'm confused by this change - maybe it doesn't belong in this patch?

>
>         /* Get an AV tracking structure. */
>         h_av = alloc_av();
> Index: hw/mlx4/kernel/bus/inc/cq.h
> ===================================================================
> --- hw/mlx4/kernel/bus/inc/cq.h (revision 3107)
> +++ hw/mlx4/kernel/bus/inc/cq.h (working copy)
> @@ -115,12 +115,12 @@
>  };
>
>  enum {
> -       MLX4_NdisPacketTcpChecksumFailed        = 1 << 1,
> -       MLX4_NdisPacketUdpChecksumFailed        = 1 << 2,
> -       MLX4_NdisPacketIpChecksumFailed         = 1 << 3,
> -       MLX4_NdisPacketTcpChecksumSucceeded = 1 << 4,
> -       MLX4_NdisPacketUdpChecksumSucceeded = 1 << 5,
> -       MLX4_NdisPacketIpChecksumSucceeded      = 1 << 6
> +       MLX4_NdisPacketTcpChecksumFailed        = 1 << 0,
> +       MLX4_NdisPacketUdpChecksumFailed        = 1 << 1,
> +       MLX4_NdisPacketIpChecksumFailed         = 1 << 2,
> +       MLX4_NdisPacketTcpChecksumSucceeded = 1 << 3,
> +       MLX4_NdisPacketUdpChecksumSucceeded = 1 << 4,
> +       MLX4_NdisPacketIpChecksumSucceeded      = 1 << 5
>  };
>
>  static inline void mlx4_cq_arm(struct mlx4_cq *cq, u32 cmd,
> Index: hw/mthca/kernel/mthca_cq.c
> ===================================================================
> --- hw/mthca/kernel/mthca_cq.c  (revision 3108)
> +++ hw/mthca/kernel/mthca_cq.c  (working copy)
> @@ -118,12 +118,12 @@
>  };
>
>  enum {
> -       MTHCA_NdisPacketTcpChecksumFailed               = 1 << 1,
> -       MTHCA_NdisPacketUdpChecksumFailed               = 1 << 2,
> -       MTHCA_NdisPacketIpChecksumFailed                = 1 << 3,
> -       MTHCA_NdisPacketTcpChecksumSucceeded    = 1 << 4,
> -       MTHCA_NdisPacketUdpChecksumSucceeded    = 1 << 5,
> -       MTHCA_NdisPacketIpChecksumSucceeded     = 1 << 6
> +       MTHCA_NdisPacketTcpChecksumFailed               = 1 << 0,
> +       MTHCA_NdisPacketUdpChecksumFailed               = 1 << 1,
> +       MTHCA_NdisPacketIpChecksumFailed                = 1 << 2,
> +       MTHCA_NdisPacketTcpChecksumSucceeded    = 1 << 3,
> +       MTHCA_NdisPacketUdpChecksumSucceeded    = 1 << 4,
> +       MTHCA_NdisPacketIpChecksumSucceeded     = 1 << 5
>  };

For the HCA driver fields, why not just use the NDIS_TCP_IP_CHECKSUM_PACKET_INFO rather than making your own enum?  Is there an issue including ndis.h in the HCA driver?

>  struct mthca_cqe {
> Index: inc/iba/ib_types.h
> ===================================================================
> --- inc/iba/ib_types.h  (revision 3107)
> +++ inc/iba/ib_types.h  (working copy)
> @@ -11395,3 +11395,4 @@
>
>
>
> +

This change probably doesn't belong in this patch.

> Index: ulp/ipoib/kernel/ipoib_port.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.c       (revision 3107)
> +++ ulp/ipoib/kernel/ipoib_port.c       (working copy)
> @@ -2542,6 +2542,8 @@
>         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 );
> @@ -2621,21 +2623,30 @@
>                         ("__buf_mgr_get_ndis_pkt failed\n") );
>                 return IB_INSUFFICIENT_RESOURCES;
>         }
> -       if (p_port->p_adapter->params.recv_chksum_offload !=
> CSUM_BYPASS) {
> -       /* Get the checksums directly from packet information. */
> -       /* In this case, no one of cheksum's cat get false value */
> -       /* If hardware checksum failed or wasn't calculated, NDIS will
> recalculate it again */
> +
> +       chksum.Value = 0;
> +       switch (p_port->p_adapter->params.recv_chksum_offload) {
> +         case CSUM_DISABLED:
> +               NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> +               (void*)(uintn_t)chksum.Value;
> +               break;
> +         case CSUM_ENABLED:
> +               /* Get the checksums directly from packet information.
> */
> +               /* In this case, no one of cheksum's cat get false
> value
> */
> +               /* If hardware checksum failed or wasn't calculated,
> NDIS will recalculate it again */
>                 NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> -                       (PVOID) (uintn_t) (p_desc->ndis_csum.Value);
> -       } else {
> -               NDIS_TCP_IP_CHECKSUM_PACKET_INFO        chksum;
> +                       (void*)(uintn_t)(p_desc->ndis_csum.Value);
> +               break;
> +         case CSUM_BYPASS:
>                 /* 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;
> +               break;
> +         default:
> +               ASSERT(FALSE);

Why not do this check when you store p_desc->ndis_csum?  Also, the default case should probably fall through to the CSUM_DISABLED case after asserting so that a release build at least gets the correct behavior.

>         }
>         ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len );