On 20.06.2018 21:12, Ferruh Yigit wrote:
On 6/20/2018 6:39 PM, Andrew Rybchenko wrote:
On 06/20/2018 08:24 PM, Ferruh Yigit wrote:
On 6/20/2018 8:42 AM, Andrew Rybchenko wrote:
On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
CRC should advertise this offload capability.
DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
default behavior in PMDs are to keep the CRC until this flag removed
Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
- Setting both KEEP_CRC & CRC_STRIP is INVALID
- Setting only CRC_STRIP PMD should strip the CRC
- Setting only KEEP_CRC PMD should keep the CRC
- Not setting both PMD should keep the CRC
A helper function rte_eth_dev_is_keep_crc() has been added to be able to
change the no flag behavior with minimal changes in PMDs.
The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
remove rte_eth_dev_is_keep_crc() checks next release, related code
commented to help the maintenance task.
And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
they don't use CRC at all, when an application requires this offload
virtual PMDs should not return error.
Signed-off-by: Ferruh Yigit <ferruh.yi...@intel.com>
---
<...>
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
b/lib/librte_ethdev/rte_ethdev_driver.h
index c9c825e3f..09a42f8c2 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
int __rte_experimental
rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t
ethdev_uninit);
+/**
+ * PMD helper function to check if keeping CRC is requested
+ *
+ * @param rx_offloads
+ * offloads variable
+ *
+ * @return
+ * Return positive if keeping CRC is requested,
+ * zero if stripping CRC is requested
+ */
+static inline int
+rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
+{
+ if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
+ return 0;
+
+ /* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
+ return 1;
+}
+
#ifdef __cplusplus
}
#endif
A couple of control questions about the function:
- shouldn't __rte_experimental be used?
This is an internal function, not API, so I think doesn't require to be
experimental.
Just to make my thoughts clear: description does not say that it is an internal.
So, nothing prevents external entities to use it. Changes will be API breakage.
rte_ethdev_driver.h is not public header, it is just for PMDs.
I see. So, it will not be a problem to remove it. OK.
- if the function remains in the future, it will be a bit asymmetric vs other
offload flags. Right now it is clear why the function is introduced, but
it is the question if the function should remain or go away in the future
(as far as I know no other offload flag has similar function to check).
No other offloads don't have similar functions, this is kind special.
There will be more changes related CRC next release, CRC_STRIP will be removed
and no flag will mean strip CRC. So the conditions to is_keep_crc will be
changed.
This function is to manage this change easier, localize the information in to
single function to make it easy to update later.
It is perfectly clear why it is required right now and introduced (as I said
from the very beginning).
Yes, it is will be the history which explains why it is so, but if we make
a step forward and discard the history it will look asymmetric -
it will be a function which checks single bit. It is really minor and
100% up to you.
I see, right it will be just a wrapper to bit check. In this patch it helps to
revert to logic, from strip_crc to keep_crc. In next release I am OK to remove
function and return back to bit check in PMDs, will this be more reasonable?
Just for consistency I'd say yes.
