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

[Shahaf Shuler <shahafs@mellanox.com>]

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. 

> 
> Are you aware of any use case that is broken with option 2?
> 
> 
> >
> >>
> >>> 2. there is a conversion function on ethdev. Which for example
> >>> converts
> >> ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the
> PMDs.
> >>> 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and
> >> remove of the conversion functions.
> >>
> >>
> >>
> >