-----Original Message-----
From: Salil Mehta
Sent: Thursday, December 01, 2016 12:09 PM
To: 'David Miller'
Cc: Zhuangyuzeng (Yisen); mehta.salil.lnk@gmail.com;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
Subject: RE: [PATCH V2 net-next] net: hns: Fix to conditionally convey
RX checksum flag to stack

quoted

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, November 30, 2016 7:26 PM
To: Salil Mehta
Cc: Zhuangyuzeng (Yisen); mehta.salil.lnk@gmail.com;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
Subject: Re: [PATCH V2 net-next] net: hns: Fix to conditionally

convey

quoted

RX checksum flag to stack

From: Salil Mehta <redacted>
Date: Tue, 29 Nov 2016 13:09:45 +0000

quoted

+	/* We only support checksum for IPv4,UDP(over IPv4 or IPv6),

TCP(over

quoted

+	 * IPv4 or IPv6) and SCTP but we support many L3(IPv4, IPv6,

MPLS,

quoted

+	 * PPPoE etc) and L4(TCP, UDP, GRE, SCTP, IGMP, ICMP etc.)

protocols.

quoted

+	 * We want to filter out L3 and L4 protocols early on for which

checksum

quoted

+	 * is not supported.

 ...

quoted

+	 */
+	l3id = hnae_get_field(flag, HNS_RXD_L3ID_M, HNS_RXD_L3ID_S);
+	l4id = hnae_get_field(flag, HNS_RXD_L4ID_M, HNS_RXD_L4ID_S);
+	if ((l3id != HNS_RX_FLAG_L3ID_IPV4) &&
+	    ((l3id != HNS_RX_FLAG_L3ID_IPV6) ||
+	     (l4id != HNS_RX_FLAG_L4ID_UDP)) &&
+	    ((l3id != HNS_RX_FLAG_L3ID_IPV6) ||
+	     (l4id != HNS_RX_FLAG_L4ID_TCP)) &&
+	    (l4id != HNS_RX_FLAG_L4ID_SCTP))
+		return;

I have a hard time understanding this seemingly overcomplicated
check.

It looks like if L3 is IPV4 it will accept any underlying L4

protocol,

quoted

but is that what is really intended?  That doesn't match what this

new

quoted

comment states.

I agree that it is bit difficult to read. Earlier, I was banking on the
register(mistakenly, its hardware implementation err ) to de-multiplex
the checksum error type. The register supported indication of just IPv4
Header Checksum Error as well (which meant it could carry any L4
protocol).
IPv6 does not have similar Header checksum error. Therefore, to check
if it is just IPv4 Header checksum error or any supported L4 transport
(UDP or TCP) error over IPv4 or IPv6 I had to bank upon above complex
check

Below suggested solution check would have been insufficient for
example, if packet had IPv4/IGMP and there was a checksum error in IPv4
header.

Comment states:
" We only support checksum for IPv4, UDP(over IPv4 or IPv6),
  TCP(over IPv4 or IPv6) and SCTP"
   1) Checksum of IPv4 (IPv4 header)
   2) UDP(over IPv4 or IPv6)
   3) TCP(over IPv4 or IPv6)
   4) SCTP (over IPv4 or IPv6)*

(*) I should have put IPv4/IPv6 check for SCTP in the code
    and made it clear in the comment as well?

quoted

My understanding is that the chip supports checksums for:

	UDP/IPV4
	UDP/IPV6
	TCP/IPV4
	TCP/IPV6
	SCTP/IPV4
	SCTP/IPV6

Hardware also supports checksum of IPv4 Header.

quoted

So the simplest thing is to validate each level one at a time:

	if (l3 != IPV4 && l3 != IPV6)
		return;
	if (l4 != UDP && l4 != TCP && l4 != SCTP)
		return;

I guess above check will fail to catch cases like IPv4/IGMP, when there
is a bad checksum in The IPv4 header.

But maybe now since we don't have any method to de-multiplex the kind
of
checksum error (cannot depend upon register) we can have below code
re-arrangement:

hns_nic_rx_checksum() {
      /* check supported L3 protocol */
	if (l3 != IPV4 && l3 != IPV6)
		return;
      /* check if L3 protocols error */
      if (l3e)
	 	return;

      /* check if the packets are fragmented */
	If (l3frags)
		Return;

      /* check supported L4 protocol */
 	if (l4 != UDP && l4 != TCP && l4 != SCTP)
 		return;

Hi David,
This logic might not do as well since IGMP packet with valid IP
Checksum will be bypassed here. We would want to set
CHECKSUM_UNNECESSARY for such IP packets as well.

It looks to me the cumbersome check in the PATCH V2 should
be retained.

      /* check if any L4 protocol error */
      if (l3e)
	 	return;

      /* packet with valid checksum - covey to stack */
      skb->ip_summed = CHECKSUM_UNNECESSARY
}

Hope I am not missing something here. Please correct my understanding
if there is any gap here. Thanks!

Best regards
Salil