Hi Pavan,
thanks for the patch. Please, see my review notes below.
On 10/29/19 8:03 AM, pbhagavat...@marvell.com wrote:
From: Pavan Nikhilesh <pbhagavat...@marvell.com>
Some PMD can't work when certain offloads are disabled, to work around
this the PMD auto enable the offloads internally and expose it through
dev->data->dev_conf.rxmode.offloads.
After dev_configure is called compare the requested offloads to the
enabled offloads and log any offloads that have been enabled by the PMD.
Suggested-by: Andrew Rybchenko <arybche...@solarflare.com>
Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
---
lib/librte_ethdev/rte_ethdev.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index fef1dbb61..7dfc2f691 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1142,6 +1142,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q,
uint16_t nb_tx_q,
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
struct rte_eth_conf orig_conf;
+ uint64_t offloads_force_ena;
+ uint64_t offload;
int diag;
int ret;
@@ -1357,6 +1359,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
goto rollback;
}
+ /* Extract Rx offload bits that can't be disabled and log them */
+ offloads_force_ena = dev_conf->rxmode.offloads ^
+ dev->data->dev_conf.rxmode.offloads;
Strictly speaking XOR returns diff and in theory the diff could
catch requested but not enabled offload in fact.
So, I think the variable name should be offloads_diff.
Yes, it is unexpected and very bad, but it adds even more
value to the check.
May be requested but not enabled offloads should be checked first:
((dev_conf->rxmode.offloads & ~dev->data->dev_conf.rxmode.offloads) != 0)
but I think it would be useful to log these offloads as well to help
debugging,
so, it should be handled below.
+ while (__builtin_popcount(offloads_force_ena)) {
If we really need it, __builtin_popcountll() should be used, but I think we
don't need it here in fact since all we want to know if offloads_diff is
0 or not.
So, comparison to 0 will do the job without any builtins.
+ offload = 1ULL << __builtin_ctzll(offloads_force_ena);
+ offloads_force_ena &= ~offload;
Below we should differentiate if the offload is requested but not
enabled (error)
and if the offload is not requested but enabled (info or warning as Ferruh
suggested).
I think ret should be set to some error if we find any requested, but not
enabled offload and finally configure should fail (after logging of all
violations) since it is a strong violation.
Same for Tx below.
Also I think that it is better to factor out these checks into a separate
function since rte_eth_dev_configure() is already too long.
It looks like that parameters should port ID, requested and
result offloads.
+ RTE_ETHDEV_LOG(INFO, "Port %u can't disable Rx offload %s\n",
+ port_id, rte_eth_dev_rx_offload_name(offload));
+ }
+
+ /* Extract Tx offload bits that can't be disabled and log them */
+ offloads_force_ena = dev_conf->txmode.offloads ^
+ dev->data->dev_conf.txmode.offloads;
+ while (__builtin_popcount(offloads_force_ena)) {
+ offload = 1ULL << __builtin_ctzll(offloads_force_ena);
+ offloads_force_ena &= ~offload;
+ RTE_ETHDEV_LOG(INFO, "Port %u can't disable Tx offload %s\n",
+ port_id, rte_eth_dev_tx_offload_name(offload));
+ }
+
return 0;
rollback:
