Re: [dpdk-dev] [EXT] Re: [PATCH v15 3/7] ethdev: add validation to offloads set by PMD

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.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.
>

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