Re: [PATCH v2 1/2] ethdev: add API to check queue ID validity

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 5/22/23 16:09, Dengdui Huang wrote:
> The API rte_eth_dev_is_valid_rxq/txq checks
> the port ID validity and then the Rx/Tx queue ID is valid.

What is valid Tx/Rx queue? It depends on on caller
expectations. Some functions are satisfied with just
check vs configured number of queues. Some require
the queue to be setup. May be some should require
the queue to be started.

So, I suggest to avoid term "valid" and be more precise
here and API naming.

> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---
>   doc/guides/rel_notes/release_23_07.rst |  5 ++++
>   lib/ethdev/rte_ethdev.c                | 30 +++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h                | 36 ++++++++++++++++++++++++++
>   lib/ethdev/version.map                 |  4 +++
>   4 files changed, 75 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_23_07.rst b/doc/guides/rel_notes/release_23_07.rst
> index a9b1293689..19e645156f 100644
> --- a/doc/guides/rel_notes/release_23_07.rst
> +++ b/doc/guides/rel_notes/release_23_07.rst
> @@ -56,6 +56,11 @@ New Features
>        =======================================================
>   
>   
> +* **Added ethdev Rx/Tx queue id check API.**
> +
> +  Added ethdev Rx/Tx queue id check API which provides functions

id -> ID

> +  for check if Rx/Tx queue id is valid.

id -> ID

> +

It should be two empty lines here and just one above.

>   Removed Items
>   -------------
>   
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 4d03255683..3d85218127 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -407,6 +407,36 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
>   	return is_valid;
>   }
>   
> +int
> +rte_eth_dev_is_valid_rxq(uint16_t port_id, uint16_t queue_id)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (queue_id >= dev->data->nb_rx_queues ||
> +			dev->data->rx_queues[queue_id] == NULL)

We already have internal eth_dev_validate_tx_queue(). Shouldn't
it be used here?

Also, some functions check that queues array is not NULL.
If the the is excessive after queue number check, it
should be consistent everywhere and corresponding check
of the array pointer vs NULL should be removed in a separate
cleanup patch. If the check is required in some corner cases
(I hope no), it should be here as well.

> +		return -EINVAL;
> +
> +	return 0;
> +}

[snip]