On Fri, 2021-09-17 at 14:53 +0300, Andrew Rybchenko wrote:

On 9/17/21 2:29 PM, Andrew Rybchenko wrote:

quoted

On 9/17/21 12:39 PM, Xueming Li wrote:

quoted

Some drivers don't need Rx and Tx queue release callback, make it
optional.

Signed-off-by: Xueming Li <redacted>

LGTM, but please, consider one nit below

Reviewed-by: Andrew Rybchenko <redacted>

quoted

---
 lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index daf5ca9242..2f316d1646 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c

@@ -905,12 +905,11 @@ eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 			return -(ENOMEM);
 		}
 	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
-
 		rxq = dev->data->rx_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+		if (dev->dev_ops->rx_queue_release != NULL)
+			for (i = nb_queues; i < old_nb_queues; i++)
+				(*dev->dev_ops->rx_queue_release)(rxq[i]);

Since 'if' body has more than one line, I'd add curly brackets
around to make it a bit easier to read and more robust against
future changes.

Similar note is applicable to many similar cases in the patch.

Reviewed the next patch I realize one thing:
Who is responsible for setting dev->data->rxq[rx_queue_id] to
NULL if release callback is not specified. IMHO, it is
inconsistent to keep it filled in after release. I think that
the generic code in ethdev must care about it regardless
callback presence. It means that we need helper function
which cares about it in single place. Also it means that
we can't optimize loops like this. We need the loop anyway
to set all queue pointers to NULL.

Yes, a dedicate function make things more clear, thanks! updated in v4.