From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752431AbdGEI2b (ORCPT ); Wed, 5 Jul 2017 04:28:31 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:9355 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750841AbdGEI23 (ORCPT ); Wed, 5 Jul 2017 04:28:29 -0400 Subject: Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description To: David Miller CC: , , , , , , , , , , , References: <1499165253-184543-1-git-send-email-linyunsheng@huawei.com> <1499165253-184543-2-git-send-email-linyunsheng@huawei.com> <20170704.032833.2030036660842556798.davem@davemloft.net> From: Yunsheng Lin Message-ID: <3fca563b-b6c5-962a-f843-d8115a83cb5a@huawei.com> Date: Wed, 5 Jul 2017 16:28:13 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170704.032833.2030036660842556798.davem@davemloft.net> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.74.190.125] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0B0203.595CA325.0058,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 4cec879e99922515409210db70ad421a Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, David On 2017/7/4 18:28, David Miller wrote: > From: Lin Yun Sheng > Date: Tue, 4 Jul 2017 18:47:31 +0800 > >> From: Yunsheng Lin >> >> 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 >> Reviewed-by: Daode Huang >> Reviewed-by: Yunsheng Lin > > 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