[PATCH 0/5] centralize deferred start checks

[PATCH 0/5] centralize deferred start checks

[Stephen Hemminger]

Recent zxdh driver review raised the question of why should
drivers have to check rx_conf for deferred start support.
This can be better handled across all drivers at ethdev level.

Stephen Hemminger (5):
  ethdev: check that device supports deferred start
  net/dpaa: remove unnecessary deferred start check
  net/dpaa2: remove unneeded deferred start check
  net/enetfec: remove unneeded deferred start check
  net/virtio: remove unneeded deferred start check

 drivers/net/dpaa/dpaa_ethdev.c    | 10 ----------
 drivers/net/dpaa2/dpaa2_ethdev.c  | 14 --------------
 drivers/net/enetfec/enet_ethdev.c | 12 ------------
 drivers/net/virtio/virtio_rxtx.c  | 10 ----------
 lib/ethdev/rte_ethdev.c           | 10 ++++++++++
 5 files changed, 10 insertions(+), 46 deletions(-)

-- 
2.45.2

[PATCH 1/5] ethdev: check that device supports deferred start

[Stephen Hemminger]

The check for supporting deferred start should be handled at
the ethdev level for all devices.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ethdev/rte_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b..7768058f6d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2264,6 +2264,11 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	if (rx_conf != NULL)
 		rx_offloads |= rx_conf->offloads;
 
+	/* Deferred start requires that device supports queue start */
+	if (rx_conf != NULL && rx_conf->rx_deferred_start &&
+	    *dev->dev_ops->rx_queue_start == NULL)
+		return -ENOTSUP;
+
 	/* Ensure that we have one and only one source of Rx buffers */
 	if ((mp != NULL) +
 	    (rx_conf != NULL && rx_conf->rx_nseg > 0) +
@@ -2575,6 +2580,11 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Deferred start requires that device supports queue start */
+	if (tx_conf != NULL && tx_conf->tx_deferred_start &&
+	    *dev->dev_ops->tx_queue_start == NULL)
+		return -ENOTSUP;
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		return ret;
-- 
2.45.2

[PATCH 2/5] net/dpaa: remove unnecessary deferred start check

[Stephen Hemminger]

This check is now done in ethdev layer.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa/dpaa_ethdev.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index e8d34e5898..ce4bbbb8f0 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -1089,11 +1089,6 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 		return -rte_errno;
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA_PMD_ERR("%p:Rx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	rxq->nb_desc = UINT16_MAX;
 	rxq->offloads = rx_conf->offloads;
 
@@ -1399,11 +1394,6 @@ int dpaa_eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA_PMD_ERR("%p:Tx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	txq->nb_desc = UINT16_MAX;
 	txq->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH 3/5] net/dpaa2: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index a9bce854c3..8624bba0ce 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -726,13 +726,6 @@ dpaa2_dev_rx_queue_setup(struct rte_eth_dev *dev,
 		DPAA2_PMD_WARN("To use Normal buffers, run 'export DPNI_NORMAL_BUF=1' before running dynamic_dpl.sh script");
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Rx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	if (!priv->bp_list || priv->bp_list->mp != mb_pool) {
 		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 			ret = rte_dpaa2_bpid_info_init(mb_pool);
@@ -889,13 +882,6 @@ dpaa2_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Tx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	dpaa2_q->nb_desc = UINT16_MAX;
 	dpaa2_q->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH 4/5] net/enetfec: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enetfec/enet_ethdev.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
index 91c0f60490..d8c81f4a70 100644
--- a/drivers/net/enetfec/enet_ethdev.c
+++ b/drivers/net/enetfec/enet_ethdev.c
@@ -377,12 +377,6 @@ enetfec_tx_queue_setup(struct rte_eth_dev *dev,
 		sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		ENETFEC_PMD_ERR("Tx deferred start not supported");
-		return -EINVAL;
-	}
-
 	/* allocate transmit queue */
 	txq = rte_zmalloc(NULL, sizeof(*txq), RTE_CACHE_LINE_SIZE);
 	if (txq == NULL) {
@@ -456,12 +450,6 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 			sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		ENETFEC_PMD_ERR("Rx deferred start not supported");
-		return -EINVAL;
-	}
-
 	if (queue_idx >= ENETFEC_MAX_Q) {
 		ENETFEC_PMD_ERR("Invalid queue id %" PRIu16 ", max %d",
 			queue_idx, ENETFEC_MAX_Q);
-- 
2.45.2

[PATCH 5/5] net/virtio: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_rxtx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index b67f063b31..6ae3a6eb12 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -654,11 +654,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (rx_conf->rx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Rx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	buf_size = virtio_rx_mem_pool_buf_size(mp);
 	if (!virtio_rx_check_scatter(hw->max_rx_pkt_len, buf_size,
 				     hw->rx_ol_scatter, &error)) {
@@ -819,11 +814,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (tx_conf->tx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Tx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
 	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
-- 
2.45.2

[PATCH v2 0/5] add check for deferred start to ethdev

[Stephen Hemminger]

Recent zxdh driver review raised the question of why should
drivers have to check rx_conf for deferred start support.
This can be better handled across all drivers at ethdev level.

v2 - annotate now unused conf pointer in enetfec

Stephen Hemminger (5):
  ethdev: check that device supports deferred start
  net/dpaa: remove unnecessary deferred start check
  net/dpaa2: remove unneeded deferred start check
  net/enetfec: remove unneeded deferred start check
  net/virtio: remove unneeded deferred start check

 drivers/net/dpaa/dpaa_ethdev.c    | 10 ----------
 drivers/net/dpaa2/dpaa2_ethdev.c  | 14 --------------
 drivers/net/enetfec/enet_ethdev.c | 16 ++--------------
 drivers/net/virtio/virtio_rxtx.c  | 10 ----------
 lib/ethdev/rte_ethdev.c           | 10 ++++++++++
 5 files changed, 12 insertions(+), 48 deletions(-)

-- 
2.45.2

[PATCH v2 1/5] ethdev: check that device supports deferred start

[Stephen Hemminger]

The check for supporting deferred start should be handled at
the ethdev level for all devices.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ethdev/rte_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b..7768058f6d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2264,6 +2264,11 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	if (rx_conf != NULL)
 		rx_offloads |= rx_conf->offloads;
 
+	/* Deferred start requires that device supports queue start */
+	if (rx_conf != NULL && rx_conf->rx_deferred_start &&
+	    *dev->dev_ops->rx_queue_start == NULL)
+		return -ENOTSUP;
+
 	/* Ensure that we have one and only one source of Rx buffers */
 	if ((mp != NULL) +
 	    (rx_conf != NULL && rx_conf->rx_nseg > 0) +
@@ -2575,6 +2580,11 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Deferred start requires that device supports queue start */
+	if (tx_conf != NULL && tx_conf->tx_deferred_start &&
+	    *dev->dev_ops->tx_queue_start == NULL)
+		return -ENOTSUP;
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		return ret;
-- 
2.45.2

[PATCH v2 2/5] net/dpaa: remove unnecessary deferred start check

[Stephen Hemminger]

This check is now done in ethdev layer.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa/dpaa_ethdev.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index e8d34e5898..ce4bbbb8f0 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -1089,11 +1089,6 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 		return -rte_errno;
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA_PMD_ERR("%p:Rx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	rxq->nb_desc = UINT16_MAX;
 	rxq->offloads = rx_conf->offloads;
 
@@ -1399,11 +1394,6 @@ int dpaa_eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA_PMD_ERR("%p:Tx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	txq->nb_desc = UINT16_MAX;
 	txq->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH v2 3/5] net/dpaa2: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index a9bce854c3..8624bba0ce 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -726,13 +726,6 @@ dpaa2_dev_rx_queue_setup(struct rte_eth_dev *dev,
 		DPAA2_PMD_WARN("To use Normal buffers, run 'export DPNI_NORMAL_BUF=1' before running dynamic_dpl.sh script");
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Rx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	if (!priv->bp_list || priv->bp_list->mp != mb_pool) {
 		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 			ret = rte_dpaa2_bpid_info_init(mb_pool);
@@ -889,13 +882,6 @@ dpaa2_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Tx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	dpaa2_q->nb_desc = UINT16_MAX;
 	dpaa2_q->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH v2 4/5] net/enetfec: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enetfec/enet_ethdev.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
index 91c0f60490..9835532595 100644
--- a/drivers/net/enetfec/enet_ethdev.c
+++ b/drivers/net/enetfec/enet_ethdev.c
@@ -366,7 +366,7 @@ enetfec_tx_queue_setup(struct rte_eth_dev *dev,
 			uint16_t queue_idx,
 			uint16_t nb_desc,
 			unsigned int socket_id __rte_unused,
-			const struct rte_eth_txconf *tx_conf)
+			const struct rte_eth_txconf *tx_conf __rte_unused)
 {
 	struct enetfec_private *fep = dev->data->dev_private;
 	unsigned int i;
@@ -377,12 +377,6 @@ enetfec_tx_queue_setup(struct rte_eth_dev *dev,
 		sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		ENETFEC_PMD_ERR("Tx deferred start not supported");
-		return -EINVAL;
-	}
-
 	/* allocate transmit queue */
 	txq = rte_zmalloc(NULL, sizeof(*txq), RTE_CACHE_LINE_SIZE);
 	if (txq == NULL) {
@@ -443,7 +437,7 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 			uint16_t queue_idx,
 			uint16_t nb_rx_desc,
 			unsigned int socket_id __rte_unused,
-			const struct rte_eth_rxconf *rx_conf,
+			const struct rte_eth_rxconf *rx_conf __rte_unused,
 			struct rte_mempool *mb_pool)
 {
 	struct enetfec_private *fep = dev->data->dev_private;
@@ -456,12 +450,6 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 			sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		ENETFEC_PMD_ERR("Rx deferred start not supported");
-		return -EINVAL;
-	}
-
 	if (queue_idx >= ENETFEC_MAX_Q) {
 		ENETFEC_PMD_ERR("Invalid queue id %" PRIu16 ", max %d",
 			queue_idx, ENETFEC_MAX_Q);
-- 
2.45.2

[PATCH v2 5/5] net/virtio: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_rxtx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index b67f063b31..6ae3a6eb12 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -654,11 +654,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (rx_conf->rx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Rx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	buf_size = virtio_rx_mem_pool_buf_size(mp);
 	if (!virtio_rx_check_scatter(hw->max_rx_pkt_len, buf_size,
 				     hw->rx_ol_scatter, &error)) {
@@ -819,11 +814,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (tx_conf->tx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Tx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
 	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
-- 
2.45.2

Re: [PATCH v2 1/5] ethdev: check that device supports deferred start

[Andrew Rybchenko]

On 12/14/24 21:07, Stephen Hemminger wrote:
> The check for supporting deferred start should be handled at
> the ethdev level for all devices.

It is a good idea to check it on ethdev level.

Strictly speaking presence of queue start/stop callback does not mean
support for deferred start right now. It is possible to use stop/start
without deferred start feature.

However, such check is much better than nothing since deferred start
definitely requires queue start callback.

It would be good to clarify it in the documentation.
doc/guides/nics/features.rst does not mention deferred start at all.
In fact, I don't mind to couple deferred start to queue start/stop
features.

One nit below.

Anyway:
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/ethdev/rte_ethdev.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 6413c54e3b..7768058f6d 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -2264,6 +2264,11 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>   	if (rx_conf != NULL)
>   		rx_offloads |= rx_conf->offloads;
>   
> +	/* Deferred start requires that device supports queue start */
> +	if (rx_conf != NULL && rx_conf->rx_deferred_start &&
> +	    *dev->dev_ops->rx_queue_start == NULL)
> +		return -ENOTSUP;

Wouldn't it be useful to add some kind of logging to simplify
debugging in this case.

> +
>   	/* Ensure that we have one and only one source of Rx buffers */
>   	if ((mp != NULL) +
>   	    (rx_conf != NULL && rx_conf->rx_nseg > 0) +
> @@ -2575,6 +2580,11 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>   		return -EINVAL;
>   	}
>   
> +	/* Deferred start requires that device supports queue start */
> +	if (tx_conf != NULL && tx_conf->tx_deferred_start &&
> +	    *dev->dev_ops->tx_queue_start == NULL)
> +		return -ENOTSUP;
> +
>   	ret = rte_eth_dev_info_get(port_id, &dev_info);
>   	if (ret != 0)
>   		return ret;

Re: [PATCH v2 1/5] ethdev: check that device supports deferred start

[Stephen Hemminger]

On Sun, 15 Dec 2024 11:56:55 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> On 12/14/24 21:07, Stephen Hemminger wrote:
> > The check for supporting deferred start should be handled at
> > the ethdev level for all devices.  
> 
> It is a good idea to check it on ethdev level.
> 
> Strictly speaking presence of queue start/stop callback does not mean
> support for deferred start right now. It is possible to use stop/start
> without deferred start feature.

Right, there are drivers that define the callback but have no logic
in place to do deferred start. They just ignore the flag.
Drivers with this odditiy are: 
	ark, atlantic, cxgbe, enic, hinic, ipn3ke, nfb, nfp, ntnic
This patch set won't change that.

There are also some drivers which claim to support queue start/stop
in the documentation, but there is no functions:
   virtio, mana, netvsc, mlx4, vmxnet3
Will fix that in next version of this series.

> 
> However, such check is much better than nothing since deferred start
> definitely requires queue start callback.
> 
> It would be good to clarify it in the documentation.
> doc/guides/nics/features.rst does not mention deferred start at all.
> In fact, I don't mind to couple deferred start to queue start/stop
> features.

It is a bug that the drivers that do queue start/stop and don't
implement deferred start. There is no hardware reason to not support
it, just missing feature during driver development.

> 
> One nit below.
> 
> Anyway:
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[PATCH v3 0/6] queue start/stop and deferred checks

[Stephen Hemminger]

Recent zxdh driver review raised the question of why should
drivers have to check rx_conf for deferred start support.
This can be better handled across all drivers at ethdev level.

Also found some drivers were incorrect in feature flags about
handling of queue start/stop.

v3 - fix doc about queue start/stop for some drivers

Stephen Hemminger (6):
  ethdev: check that device supports deferred start
  doc: fix feature flags for queue start/stop
  net/dpaa: remove unnecessary deferred start check
  net/dpaa2: remove unneeded deferred start check
  net/enetfec: remove unneeded deferred start check
  net/virtio: remove unneeded deferred start check

 doc/guides/nics/features/gve.ini     |  1 +
 doc/guides/nics/features/mana.ini    |  1 -
 doc/guides/nics/features/netvsc.ini  |  1 -
 doc/guides/nics/features/virtio.ini  |  1 -
 doc/guides/nics/features/vmxnet3.ini |  1 -
 drivers/net/dpaa/dpaa_ethdev.c       | 10 ----------
 drivers/net/dpaa2/dpaa2_ethdev.c     | 14 --------------
 drivers/net/enetfec/enet_ethdev.c    | 16 ++--------------
 drivers/net/virtio/virtio_rxtx.c     | 10 ----------
 lib/ethdev/rte_ethdev.c              | 16 ++++++++++++++++
 10 files changed, 19 insertions(+), 52 deletions(-)

-- 
2.45.2

[PATCH v3 1/6] ethdev: check that device supports deferred start

[Stephen Hemminger]

The check for supporting deferred start should be handled at
the ethdev level for all devices.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b..7678fa47b6 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2264,6 +2264,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	if (rx_conf != NULL)
 		rx_offloads |= rx_conf->offloads;
 
+	/* Deferred start requires that device supports queue start */
+	if (rx_conf != NULL && rx_conf->rx_deferred_start &&
+	    *dev->dev_ops->rx_queue_start == NULL) {
+		RTE_ETHDEV_LOG_LINE(ERR,
+			    "Deferred start requested, but driver does not support queue start");
+		return -ENOTSUP;
+	}
+
 	/* Ensure that we have one and only one source of Rx buffers */
 	if ((mp != NULL) +
 	    (rx_conf != NULL && rx_conf->rx_nseg > 0) +
@@ -2575,6 +2583,14 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Deferred start requires that device supports queue start */
+	if (tx_conf != NULL && tx_conf->tx_deferred_start &&
+	    *dev->dev_ops->tx_queue_start == NULL) {
+		RTE_ETHDEV_LOG_LINE(ERR,
+			    "Deferred start requested, but driver does not support queue start");
+		return -ENOTSUP;
+	}
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		return ret;
-- 
2.45.2

[PATCH v3 2/6] doc: fix feature flags for queue start/stop

[Stephen Hemminger]

Several drivers have mismatch between the ethdev ops for queue
start/stop and the documentation. The gve driver does implement
queue start/stop support. The mana, netvsc, virtio and vmxnet3
drivers do not implement the rx_queue_start callback.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 doc/guides/nics/features/gve.ini     | 1 +
 doc/guides/nics/features/mana.ini    | 1 -
 doc/guides/nics/features/netvsc.ini  | 1 -
 doc/guides/nics/features/virtio.ini  | 1 -
 doc/guides/nics/features/vmxnet3.ini | 1 -
 5 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/doc/guides/nics/features/gve.ini b/doc/guides/nics/features/gve.ini
index 8dfa229bb0..f18b829eda 100644
--- a/doc/guides/nics/features/gve.ini
+++ b/doc/guides/nics/features/gve.ini
@@ -6,6 +6,7 @@
 [Features]
 Speed capabilities   = Y
 Link status          = Y
+Queue start/stop     = Y
 MTU update           = Y
 TSO                  = Y
 RSS hash             = Y
diff --git a/doc/guides/nics/features/mana.ini b/doc/guides/nics/features/mana.ini
index 42fd3327d2..db555ffe27 100644
--- a/doc/guides/nics/features/mana.ini
+++ b/doc/guides/nics/features/mana.ini
@@ -8,7 +8,6 @@ Speed capabilities   = P
 Link status          = P
 Removal event        = Y
 Rx interrupt         = Y
-Queue start/stop     = Y
 RSS hash             = Y
 L3 checksum offload  = Y
 L4 checksum offload  = Y
diff --git a/doc/guides/nics/features/netvsc.ini b/doc/guides/nics/features/netvsc.ini
index de8c698184..32df77a03e 100644
--- a/doc/guides/nics/features/netvsc.ini
+++ b/doc/guides/nics/features/netvsc.ini
@@ -7,7 +7,6 @@
 Speed capabilities   = P
 Link status          = Y
 Free Tx mbuf on demand = Y
-Queue start/stop     = Y
 Scattered Rx         = Y
 Promiscuous mode     = Y
 Allmulticast mode    = Y
diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
index a5eab4932f..f0a1a564f9 100644
--- a/doc/guides/nics/features/virtio.ini
+++ b/doc/guides/nics/features/virtio.ini
@@ -8,7 +8,6 @@ Speed capabilities   = P
 Link status          = Y
 Link status event    = Y
 Rx interrupt         = Y
-Queue start/stop     = Y
 Scattered Rx         = P
 Promiscuous mode     = Y
 Allmulticast mode    = Y
diff --git a/doc/guides/nics/features/vmxnet3.ini b/doc/guides/nics/features/vmxnet3.ini
index 971695fc4a..749887d642 100644
--- a/doc/guides/nics/features/vmxnet3.ini
+++ b/doc/guides/nics/features/vmxnet3.ini
@@ -7,7 +7,6 @@
 Speed capabilities   = P
 Link status          = Y
 Link status event    = Y
-Queue start/stop     = Y
 MTU update           = Y
 LRO                  = Y
 TSO                  = Y
-- 
2.45.2

[PATCH v3 3/6] net/dpaa: remove unnecessary deferred start check

[Stephen Hemminger]

This check is now done in ethdev layer.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa/dpaa_ethdev.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index e8d34e5898..ce4bbbb8f0 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -1089,11 +1089,6 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 		return -rte_errno;
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA_PMD_ERR("%p:Rx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	rxq->nb_desc = UINT16_MAX;
 	rxq->offloads = rx_conf->offloads;
 
@@ -1399,11 +1394,6 @@ int dpaa_eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA_PMD_ERR("%p:Tx deferred start not supported", (void *)dev);
-		return -EINVAL;
-	}
 	txq->nb_desc = UINT16_MAX;
 	txq->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH v3 4/6] net/dpaa2: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index a9bce854c3..8624bba0ce 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -726,13 +726,6 @@ dpaa2_dev_rx_queue_setup(struct rte_eth_dev *dev,
 		DPAA2_PMD_WARN("To use Normal buffers, run 'export DPNI_NORMAL_BUF=1' before running dynamic_dpl.sh script");
 	}
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Rx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	if (!priv->bp_list || priv->bp_list->mp != mb_pool) {
 		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 			ret = rte_dpaa2_bpid_info_init(mb_pool);
@@ -889,13 +882,6 @@ dpaa2_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		DPAA2_PMD_ERR("%s:Tx deferred start not supported",
-			dev->data->name);
-		return -EINVAL;
-	}
-
 	dpaa2_q->nb_desc = UINT16_MAX;
 	dpaa2_q->offloads = tx_conf->offloads;
 
-- 
2.45.2

[PATCH v3 5/6] net/enetfec: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enetfec/enet_ethdev.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
index 91c0f60490..9835532595 100644
--- a/drivers/net/enetfec/enet_ethdev.c
+++ b/drivers/net/enetfec/enet_ethdev.c
@@ -366,7 +366,7 @@ enetfec_tx_queue_setup(struct rte_eth_dev *dev,
 			uint16_t queue_idx,
 			uint16_t nb_desc,
 			unsigned int socket_id __rte_unused,
-			const struct rte_eth_txconf *tx_conf)
+			const struct rte_eth_txconf *tx_conf __rte_unused)
 {
 	struct enetfec_private *fep = dev->data->dev_private;
 	unsigned int i;
@@ -377,12 +377,6 @@ enetfec_tx_queue_setup(struct rte_eth_dev *dev,
 		sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Tx deferred start is not supported */
-	if (tx_conf->tx_deferred_start) {
-		ENETFEC_PMD_ERR("Tx deferred start not supported");
-		return -EINVAL;
-	}
-
 	/* allocate transmit queue */
 	txq = rte_zmalloc(NULL, sizeof(*txq), RTE_CACHE_LINE_SIZE);
 	if (txq == NULL) {
@@ -443,7 +437,7 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 			uint16_t queue_idx,
 			uint16_t nb_rx_desc,
 			unsigned int socket_id __rte_unused,
-			const struct rte_eth_rxconf *rx_conf,
+			const struct rte_eth_rxconf *rx_conf __rte_unused,
 			struct rte_mempool *mb_pool)
 {
 	struct enetfec_private *fep = dev->data->dev_private;
@@ -456,12 +450,6 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 			sizeof(struct bufdesc);
 	unsigned int dsize_log2 = rte_fls_u64(dsize) - 1;
 
-	/* Rx deferred start is not supported */
-	if (rx_conf->rx_deferred_start) {
-		ENETFEC_PMD_ERR("Rx deferred start not supported");
-		return -EINVAL;
-	}
-
 	if (queue_idx >= ENETFEC_MAX_Q) {
 		ENETFEC_PMD_ERR("Invalid queue id %" PRIu16 ", max %d",
 			queue_idx, ENETFEC_MAX_Q);
-- 
2.45.2

[PATCH v3 6/6] net/virtio: remove unneeded deferred start check

[Stephen Hemminger]

Already checked in ethdev now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_rxtx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index b67f063b31..6ae3a6eb12 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -654,11 +654,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (rx_conf->rx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Rx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	buf_size = virtio_rx_mem_pool_buf_size(mp);
 	if (!virtio_rx_check_scatter(hw->max_rx_pkt_len, buf_size,
 				     hw->rx_ol_scatter, &error)) {
@@ -819,11 +814,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (tx_conf->tx_deferred_start) {
-		PMD_INIT_LOG(ERR, "Tx deferred start is not supported");
-		return -EINVAL;
-	}
-
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
 	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
-- 
2.45.2

Re: [PATCH v2 1/5] ethdev: check that device supports deferred start

[Andrew Rybchenko]

On 12/16/24 21:58, Stephen Hemminger wrote:
> On Sun, 15 Dec 2024 11:56:55 +0300
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
> 
>> On 12/14/24 21:07, Stephen Hemminger wrote:
>>> The check for supporting deferred start should be handled at
>>> the ethdev level for all devices.
>>
>> It is a good idea to check it on ethdev level.
>>
>> Strictly speaking presence of queue start/stop callback does not mean
>> support for deferred start right now. It is possible to use stop/start
>> without deferred start feature.
> 
> Right, there are drivers that define the callback but have no logic
> in place to do deferred start. They just ignore the flag.
> Drivers with this odditiy are:
> 	ark, atlantic, cxgbe, enic, hinic, ipn3ke, nfb, nfp, ntnic
> This patch set won't change that.
> 
> There are also some drivers which claim to support queue start/stop
> in the documentation, but there is no functions:
>     virtio, mana, netvsc, mlx4, vmxnet3
> Will fix that in next version of this series.
> 
>>
>> However, such check is much better than nothing since deferred start
>> definitely requires queue start callback.
>>
>> It would be good to clarify it in the documentation.
>> doc/guides/nics/features.rst does not mention deferred start at all.
>> In fact, I don't mind to couple deferred start to queue start/stop
>> features.
> 
> It is a bug that the drivers that do queue start/stop and don't
> implement deferred start. There is no hardware reason to not support
> it, just missing feature during driver development.

I agree. Please, don't forget to add rx_deferred_start and
tx_deferred_start to queue start/stop features in
doc/guides/nics/features.rst.

>>
>> One nit below.
>>
>> Anyway:
>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>