Re: [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Thomas Monjalon <thomas@monjalon.net>]

16/04/2018 19:23, Ferruh Yigit:
> On 4/1/2018 8:10 AM, Shahaf Shuler wrote:
> > Thursday, March 29, 2018 4:32 PM, Ferruh Yigit:
> >> On 3/29/2018 8:56 AM, Shahaf Shuler wrote:
> >>> Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
> >>>> 29/03/2018 07:38, Shahaf Shuler:
> >>>>> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> >>>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> >>>
> >>> Logic should be :
> >>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> >>>  - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev.
> >>>  - Setting only CRC_STRIP PMD should strip the CRC
> >>>  - Setting only KEEP_CRC PMD should keep the CRC
> >>>  - Not setting both PMD should *not* strip the CRC
> >>
> >> Hi Shahaf,
> >>
> >> I think we have two options,
> >>
> >> 1- This is v1 of this patch and also what you suggest:
> >> v18.05:
> >> - Send deprecation notice
> >>
> >> v18.08:
> >> - Add KEEP_CRC flag
> >> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> > 
> > I think the above line ...
> > 
> >>
> >> v18.11:
> >> - Remove the CRC_STRIP flag completely
> > 
> > Should be here.
> > 
> > It is better to change the default behavior only once the STRIP_CRC flags is removed. 
> > Without it on v18.08 you break application that wants to have the CRC, and on v18.11 you break the application which actually used it.
> > It is better to have all the application changes in one release - 18.11. 
> > 
> >>
> >> I think this is more proper but takes more time.
> > 
> > Me too. 
> > 
> >>
> >>
> >> 2- Based on all the reality that all devices are doing CRC strip already:
> >>
> >> v18.05:
> >> - Add KEEP_CRC flag
> >> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> >>
> >> v18.08:
> >> - Remove the CRC_STRIP flag completely
> >>
> >>
> >> With option two since not setting both KEEP_CRC & CRC_STRIP will mean
> >> "strip the CRC", it won't require a change in PMDs, only we can pay attention
> >> to get new PMDs according plan.
> >>
> >> This can be more problematic for the case that application ask for keeping
> >> CRC, because the way to say this was not setting CRC_STRIP, it changed to
> >> setting KEEP_CRC without notification. This can be less problem since many
> >> PMD already doesn't support keep crc.
> > 
> > but what about those which do support? 
> > You break application which uses PMDs which support this offload for the sake of the PMD which don't have this capability. 
> > 
> > I think #1 is the clean one. 
> 
> 
> Any decision here? So will we go with first version of this patch [1]?
> 
> [1]
> https://dpdk.org/dev/patchwork/patch/36288/

Please do a v3 according to what Shahaf is proposing:

- add KEEP_CRC in 18.08 + implement in PMDs + translate ~STRIP_CRC
- remove STRIP_CRC in 18.11 + toggle default to stripping

Or, are we able to do it quickly in 18.05-rc1, and remove in 18.08
with other offloads?