On 10/29/19 6:37 PM, pbhagavat...@marvell.com wrote:
From: Pavan Nikhilesh <pbhagavat...@marvell.com>
Some PMDs cannot work when certain offloads are enable/disabled, as a
workaround PMDs auto enable/disable offloads internally and expose it
through dev->data->dev_conf.rxmode.offloads.
After device specific dev_configure is called compare the requested
offloads to the offloads exposed by the PMD and, if the PMD failed
to enable a given offload then log it and return -EINVAL from
rte_eth_dev_configure, else if the PMD failed to disable a given offload
log and continue with rte_eth_dev_configure.
Suggested-by: Andrew Rybchenko <arybche...@solarflare.com>
Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
Few minor notes below, many thanks
Reviewed-by: Andrew Rybchenko <arybche...@solarflare.com>
---
lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 3f45b9e9c..8c58da91c 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload)
return name;
}
+static int
+_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
I'm not sure about underscore in function name. May be Thomas or
Ferruh can comment.
+ uint64_t set_offloads,
+ const char *(*f)(uint64_t))
+{
+ uint64_t offloads_diff = req_offloads ^ set_offloads;
+ uint64_t offloads_req_diff, offloads_set_diff;
+ uint64_t offload;
+ uint8_t err = 0;
+
+ /* Check if any offload is advertised but not enabled. */
+ offloads_req_diff = offloads_diff & req_offloads;
+ while (offloads_req_diff) {
+ offload = 1ULL << __builtin_ctzll(offloads_req_diff);
+ offloads_req_diff &= ~offload;
+ RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload",
Offload name does not include Rx/Tx, so IPV4_CKSUM is identical
and it is required to log if it is Rx or Tx offload separately.
Sounds like one more parameter.
+ port_id, f(offload));
+ err = 1;
+ }
+
+ if (err)
+ return -EINVAL;
+
+ /* Chech if any offload couldn't be disabled. */
+ offloads_set_diff = offloads_diff & set_offloads;
+ while (offloads_set_diff) {
+ offload = 1ULL << __builtin_ctzll(offloads_set_diff);
+ offloads_set_diff &= ~offload;
+ RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload",
+ port_id, f(offload));
+ }
Consider to do it in one loop, something like:
int rc = 0;
while (offloads_diff != 0) {
offload = 1ULL << __builtin_ctzll(offloads_diff);
offloads_diff &= ~offload;
if (offload & req_offload) {
RTE_ETHDEV_LOG(INFO,
"Port %u failed to enable %s %s offload",
port_id, f(offload), rxtx);
rc = -EINVAL;
} else {
RTE_ETHDEV_LOG(INFO,
"Port %u failed to disable %s %s offload",
port_id, f(offload), rxtx);
}
}
return rc;
BTW, I'm not sure that -EINVAL is good here, but right now
can't suggest anything better.
+
+ return 0;
+}
+
int
rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
const struct rte_eth_conf *dev_conf)
@@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t
nb_rx_q, uint16_t nb_tx_q,
goto rollback;
}
+ /* Validate Rx offloads. */
+ diag = _rte_eth_dev_validate_offloads(port_id,
+ dev_conf->rxmode.offloads,
+ dev->data->dev_conf.rxmode.offloads,
+ rte_eth_dev_rx_offload_name);
+ if (diag != 0) {
+ rte_eth_dev_rx_queue_config(dev, 0);
+ rte_eth_dev_tx_queue_config(dev, 0);
May be it is a good moment to make one more rollback
label with queues release and avoid duplicating it.
+ ret = diag;
+ goto rollback;
+ }
+
+ /* Validate Tx offloads. */
+ diag = _rte_eth_dev_validate_offloads(port_id,
+ dev_conf->txmode.offloads,
+ dev->data->dev_conf.txmode.offloads,
+ rte_eth_dev_tx_offload_name);
+ if (diag != 0) {
+ rte_eth_dev_rx_queue_config(dev, 0);
+ rte_eth_dev_tx_queue_config(dev, 0);
+ ret = diag;
+ goto rollback;
+ }
+
return 0;
rollback:
--
2.17.1
