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

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 27 Nov 2024 10:32:24 +0800
Jie Hai <haijie1@huawei.com> wrote:

> 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.
> > .  

Yes a bugfix that makes hns3 behave like other drivers for now
would be best approach.