Applied to trunk and branch. Thanks Tzachi
> -----Original Message----- > From: Alex Naslednikov > Sent: Wednesday, September 10, 2008 12:20 PM > To: Alex Naslednikov; Fab Tillier; Alex Estrin; > [email protected] > Cc: Tzachi Dar > Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch? > > Attached the patch that includes Fab comments. > Index: ulp/ipoib/kernel/ipoib_port.c > =================================================================== > --- ulp/ipoib/kernel/ipoib_port.c (revision 3142) > +++ ulp/ipoib/kernel/ipoib_port.c (revision 3143) > @@ -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,32 @@ > ("__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); > + NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet, > TcpIpChecksumPacketInfo ) = > + (void*)(uintn_t)chksum.Value; > } > ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len ); > > Index: hw/mlx4/kernel/bus/inc/cq.h > =================================================================== > --- hw/mlx4/kernel/bus/inc/cq.h (revision 3142) > +++ hw/mlx4/kernel/bus/inc/cq.h (revision 3143) > @@ -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 3142) > +++ hw/mthca/kernel/mthca_cq.c (revision 3143) > @@ -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 > }; > > struct mthca_cqe { > > -----Original Message----- > From: Alex Naslednikov > Sent: Wednesday, September 10, 2008 10:20 AM > To: 'Fab Tillier'; Alex Estrin; [email protected] > Cc: Tzachi Dar > Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch? > > See inline > > -----Original Message----- > From: Fab Tillier [mailto:[EMAIL PROTECTED] > Sent: Tuesday, September 09, 2008 7:52 PM > To: Alex Naslednikov; Alex Estrin; [email protected] > Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch? > > ---Hi Xalex, > Hello Fab ! > > > > > 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? > Yes, this change is a temporarily fix of another bug in > mthca. It's not related to the 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? > It makes sense if we had dedicated field for the checsum. > But we are going to use receive_opt in order to pass the > value of csum up to IPoIB driver (see the patch of Alex > Estrin). Using this implementation, it will be very > unreadable to define additional value of > NDIS_TCP_IP_CHECKSUM_PACKET_INFO, than embedding into TCP > options, than receive it again in IPoIB driver and > transforming it back to NDIS_TCP_IP_CHECKSUM_PACKET_INFO > > > 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. > Correct > > > 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? > Our driver will report correct IPOK bit within CQE. Thus, in > order to avoid implicit receive checksum offloading, this > check forces NDIS to recalculate the checksum > > ---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. > Agree, here it's possible to fall through CSUM_DISABLED in a > case of free version. > I will resend the patch again with this change > > > > } > > ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len ); > _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
