[dpdk-stable] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[dpdk-stable] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Ferruh Yigit]

From: Wenzhuo Lu <wenzhuo.lu@intel.com>

The new configuration is stored during the rte_eth_dev_configure() API
but the API may fail. After failure stored configuration will be
invalid since it is not fully applied to the device.

We better roll the configuration back after failure.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 49 +++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8eaa5fcc7..04dff1f5e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1092,8 +1092,10 @@ 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;
 	struct rte_eth_conf local_conf = *dev_conf;
 	int diag;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1140,6 +1142,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EBUSY;
 	}
 
+	 /* Store original config, as rollback required on failure */
+	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
+
 	/* Copy the dev_conf parameter into the dev structure */
 	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
 
@@ -1151,13 +1156,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_rx_queues=%u > %u\n",
 			port_id, nb_rx_q, dev_info.max_rx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > dev_info.max_tx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_tx_queues=%u > %u\n",
 			port_id, nb_tx_q, dev_info.max_tx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that the device supports requested interrupts */
@@ -1165,13 +1172,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support lsc\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((dev_conf->intr_conf.rmv == 1) &&
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_RMV))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support rmv\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1184,13 +1193,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 				"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				dev_info.max_rx_pktlen);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		} else if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
 			RTE_ETHDEV_LOG(ERR,
 				"Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				(unsigned)ETHER_MIN_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		}
 	} else {
 		if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN ||
@@ -1209,7 +1220,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, local_conf.rxmode.offloads,
 			dev_info.rx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
 	     local_conf.txmode.offloads) {
@@ -1219,7 +1231,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, local_conf.txmode.offloads,
 			dev_info.tx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that device supports requested rss hash functions. */
@@ -1230,7 +1243,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			"Ethdev port_id=%u invalid rss_hf: 0x%"PRIx64", valid value: 0x%"PRIx64"\n",
 			port_id, dev_conf->rx_adv_conf.rss_conf.rss_hf,
 			dev_info.flow_type_rss_offloads);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1241,7 +1255,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		RTE_ETHDEV_LOG(ERR,
 			"Port%u rte_eth_dev_rx_queue_config = %d\n",
 			port_id, diag);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = rte_eth_dev_tx_queue_config(dev, nb_tx_q);
@@ -1250,7 +1265,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			"Port%u rte_eth_dev_tx_queue_config = %d\n",
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = (*dev->dev_ops->dev_configure)(dev);
@@ -1259,7 +1275,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	/* Initialize Rx profiling if enabled at compilation time. */
@@ -1269,10 +1286,16 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	return 0;
+
+rollback:
+	memcpy(&dev->data->dev_conf, &orig_conf, sizeof(dev->data->dev_conf));
+
+	return ret;
 }
 
 void
-- 
2.17.2

[dpdk-stable] [PATCH v4 2/3] ethdev: fix device info getting

[Ferruh Yigit]

From: Wenzhuo Lu <wenzhuo.lu@intel.com>

The device information cannot be gotten correctly before
the configuration is set. Because on some NICs the
information has dependence on the configuration.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Cc: stable@dpdk.org

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 35 +++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 04dff1f5e..0f01138ea 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1104,6 +1104,22 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
+	if (dev->data->dev_started) {
+		RTE_ETHDEV_LOG(ERR,
+			"Port %u must be stopped to allow configuration\n",
+			port_id);
+		return -EBUSY;
+	}
+
+	 /* Store original config, as rollback required on failure */
+	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
+
+	/*
+	 * Copy the dev_conf parameter into the dev structure.
+	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
+	 */
+	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
+
 	rte_eth_dev_info_get(port_id, &dev_info);
 
 	/* If number of queues specified by application for both Rx and Tx is
@@ -1125,29 +1141,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		RTE_ETHDEV_LOG(ERR,
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
 			nb_rx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_ETHDEV_LOG(ERR,
 			"Number of TX queues requested (%u) is greater than max supported(%d)\n",
 			nb_tx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
-	}
-
-	if (dev->data->dev_started) {
-		RTE_ETHDEV_LOG(ERR,
-			"Port %u must be stopped to allow configuration\n",
-			port_id);
-		return -EBUSY;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
-	 /* Store original config, as rollback required on failure */
-	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
-
-	/* Copy the dev_conf parameter into the dev structure */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
-
 	/*
 	 * Check that the numbers of RX and TX queues are not greater
 	 * than the maximum number of RX and TX queues supported by the
-- 
2.17.2

[dpdk-stable] [PATCH v4 3/3] ethdev: eliminate interim variable

[Ferruh Yigit]

`local_conf` variable was needed for offload conversions but no more
required. No functional difference, only interim variable eliminated.

Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0f01138ea..5f858174b 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1093,7 +1093,6 @@ 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;
-	struct rte_eth_conf local_conf = *dev_conf;
 	int diag;
 	int ret;
 
@@ -1118,7 +1117,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * Copy the dev_conf parameter into the dev structure.
 	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
 	 */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
+	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
@@ -1192,7 +1191,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * If jumbo frames are enabled, check that the maximum RX packet
 	 * length is supported by the configured device.
 	 */
-	if (local_conf.rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+	if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
 		if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
 			RTE_ETHDEV_LOG(ERR,
 				"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
@@ -1217,23 +1216,23 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	}
 
 	/* Any requested offloading must be within its device capabilities */
-	if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
-	     local_conf.rxmode.offloads) {
+	if ((dev_conf->rxmode.offloads & dev_info.rx_offload_capa) !=
+	     dev_conf->rxmode.offloads) {
 		RTE_ETHDEV_LOG(ERR,
 			"Ethdev port_id=%u requested Rx offloads 0x%"PRIx64" doesn't match Rx offloads "
 			"capabilities 0x%"PRIx64" in %s()\n",
-			port_id, local_conf.rxmode.offloads,
+			port_id, dev_conf->rxmode.offloads,
 			dev_info.rx_offload_capa,
 			__func__);
 		ret = -EINVAL;
 		goto rollback;
 	}
-	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
-	     local_conf.txmode.offloads) {
+	if ((dev_conf->txmode.offloads & dev_info.tx_offload_capa) !=
+	     dev_conf->txmode.offloads) {
 		RTE_ETHDEV_LOG(ERR,
 			"Ethdev port_id=%u requested Tx offloads 0x%"PRIx64" doesn't match Tx offloads "
 			"capabilities 0x%"PRIx64" in %s()\n",
-			port_id, local_conf.txmode.offloads,
+			port_id, dev_conf->txmode.offloads,
 			dev_info.tx_offload_capa,
 			__func__);
 		ret = -EINVAL;
-- 
2.17.2

Re: [dpdk-stable] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
> The new configuration is stored during the rte_eth_dev_configure() API
> but the API may fail. After failure stored configuration will be
> invalid since it is not fully applied to the device.
>
> We better roll the configuration back after failure.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-stable] [PATCH v4 2/3] ethdev: fix device info getting

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
> The device information cannot be gotten correctly before
> the configuration is set. Because on some NICs the
> information has dependence on the configuration.
>
> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-stable] [PATCH v4 3/3] ethdev: eliminate interim variable

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> `local_conf` variable was needed for offload conversions but no more
> required. No functional difference, only interim variable eliminated.
>
> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

I guess it will be easier to backport the fix if it is the first fix in 
the series.

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-stable] [PATCH v4 3/3] ethdev: eliminate interim variable

[Ferruh Yigit]

On 11/13/2018 11:22 AM, Andrew Rybchenko wrote:
> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>> `local_conf` variable was needed for offload conversions but no more
>> required. No functional difference, only interim variable eliminated.
>>
>> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> I guess it will be easier to backport the fix if it is the first fix in the series.

I thought this is the less important one and that is why put it as last. Didn't
want this patch cause any conflict while getting other fixes.

Which patch do you mention to be easier to backport if this is the first, this
one or others?

> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 3/3] ethdev: eliminate interim variable

[Andrew Rybchenko]

On 11/13/18 2:51 PM, Ferruh Yigit wrote:
> On 11/13/2018 11:22 AM, Andrew Rybchenko wrote:
>> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>>> `local_conf` variable was needed for offload conversions but no more
>>> required. No functional difference, only interim variable eliminated.
>>>
>>> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> I guess it will be easier to backport the fix if it is the first fix in the series.
> I thought this is the less important one and that is why put it as last. Didn't
> want this patch cause any conflict while getting other fixes.
>
> Which patch do you mention to be easier to backport if this is the first, this
> one or others?

This one. It touches lines which are moved in the previous patch.
Not really that important.

>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>

Re: [dpdk-stable] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Ferruh Yigit]

On 11/13/2018 11:19 AM, Andrew Rybchenko wrote:
> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>
>> The new configuration is stored during the rte_eth_dev_configure() API
>> but the API may fail. After failure stored configuration will be
>> invalid since it is not fully applied to the device.
>>
>> We better roll the configuration back after failure.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Series applied to dpdk-next-net/master, thanks.