>>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.
>
Yes, I guess we can pass "RX"/"Tx" as another 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;
>
We could merge the loops I though that the differentiating them would
be informative. No strong opinions we could merge them in v16.
>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.
>
Yeah we could put them under a new label.
>
>>+ 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
