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?