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