Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

[Yunsheng Lin]

Hi, David

On 2017/7/4 18:28, David Miller wrote:
> From: Lin Yun Sheng <linyunsheng@xxxxxxxxxx>
> Date: Tue, 4 Jul 2017 18:47:31 +0800
> 
>> From: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>>
>> If driver support checksum offload, should check netdev feature
>> before fill TX description and get CSUM err bit from RX
>> description. HNS driver do the check in RX derction but it doesn't
>> do the check in TX direction.
>>
>> Signed-off-by: lipeng <lipeng321@xxxxxxxxxx>
>> Reviewed-by: Daode Huang <huangdaode@xxxxxxxxxxxxx>
>> Reviewed-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> 
> This is not correct.
> 
> You should be checking the skb->checksum field to decide if you should
> offload the TX checksum of the packet or not.
> 
> Correct drivers, as far as I am aware, do not check the feature flags
> so I wonder where you got this idea from.  Always use other well
> established existing drivers as a model for how to handle things like
> this.
> 
> And this makes sense.  An SKB can have it's checksumming determination
> made first, then the netdev feature change is made afterwards.  For
> correctness you still need to TX checksum offload that SKB otherwise
> it will be emitted without a correctly computed checksum.
> 
You are right, thanks for pointing out.
driver/net/ethernet/hisilicon/hns/hns_enet.c
fill_desc:
		if (skb->ip_summed == CHECKSUM_PARTIAL) {
			protocol = skb->protocol;
			ip_offset = ETH_HLEN;

			/*if it is a SW VLAN check the next protocol*/
			if (protocol == htons(ETH_P_8021Q)) {
				ip_offset += VLAN_HLEN;
				protocol = vlan_get_protocol(skb);
				skb->protocol = protocol;
			}

			if (skb->protocol == htons(ETH_P_IP)) {
				flag_ipoffset |= 1 << HNS_TXD_L3CS_B;
				/* check for tcp/udp header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;

			} else if (skb->protocol == htons(ETH_P_IPV6)) {
				/* ipv6 has not l3 cs, check for L4 header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;
			}

			flag_ipoffset |= ip_offset << HNS_TXD_IPOFFSET_S;
		}

the hns driver has already check the skb->ip_summed, and checking the ndev->feature is
not correct.

Will remove this patch next revision.

Best Regards
Yunsheng Lin