On 9/17/21 2:29 PM, Andrew Rybchenko wrote:
> On 9/17/21 12:39 PM, Xueming Li wrote:
>> Some drivers don't need Rx and Tx queue release callback, make it
>> optional.
>>
>> Signed-off-by: Xueming Li <xuemi...@nvidia.com>
>
> LGTM, but please, consider one nit below
>
> Reviewed-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>
>> ---
>> 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.
