Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Jie Hai <haijie1@huawei.com>]

On 2024/11/27 8:16, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 17:04:15 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> From: Dengdui Huang <huangdengdui@huawei.com>
>>
>> When KEEP_CRC offload is enabled, the CRC data is still stripped
>> in following cases:
>> 1. For HIP08 network engine, the packet type is TCP and the length
>>     is less than or equal to 60B.
>> 2. For HIP09 network engine, the packet type is IP and the length
>>     is less than or equal to 60B.
>>
>> So driver has to recaculate packet CRC for this rare scenarios.
>>
>> In addition, to avoid impacting performance, KEEP_CRC is not
>> supported when NEON or SVE algorithm is used.
>>
>> Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> Acked-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Jie Hai <haijie1@huawei.com>
> 
> Changed my mind on these patches after digging deeper into
> what other drivers are doing. The proposed patches for hns3 do
> the opposite of what the consensus of drivers is.
> 
> When looking at internals, all other drivers do not include the CRC
> in the packet length calculation. It is hard to go back and determine
> the rational for this, but my assumption is that if a packet is received
> (with KEEP_CRC enabled), the application will likely want to send that
> packet to another location, and the transmit side doesn't want the CRC.
> 
> There are a couple of related driver bugs in some drivers in handling
> of the flag as well. One driver (idpf) thinks the CRC should count for the byte
> statistics. This should be clarified and fixed.
> 
> One driver (atlantic) adds a check but doesn't implement the flag; the check for
> valid offload flags is already handled by ethdev API.
> 
> Please resubmit for a later release, and can be picked up then by 24.11 stable.
> 
There is indeed much work to be done to clarify the relationship between 
keep crc and fields in mbuf.
In the current patchset, patch 1 and patch 2 are used for this purpose, 
but a bug occurs.
If CRC is not processed in the TX direction, CRC is forwarded as packet 
data.
As a result, the packet length is increased by 4.
I agree that this part should be carefully investigated before a 
decision is made.

But for patch 3,  this is a serious bug of the hns3 driver and  and is 
irrelevant to the preceding questions.

Could you please apply the patch 3 first to fix the bug of hns3 ?
I can send a single patch of hns3 driver for the next version.

> You have found an area of DPDK which is poorly documented. Will raise an
> agenda at next techboard to get a final agreement, then put that into
> the programmer's guide.
> .