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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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/

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