Re: [PATCH v10 6/7] bbdev: add queue related warning and status information

[Thomas Monjalon <thomas@monjalon.net>]

Looking at this patch because I have been alerted about the ABI compat handling.
I see some details that should have been caught in earlier reviews.

30/09/2022 20:46, Nicolas Chautru:

> +/*
> + * Maximum size to be used to manage the enum rte_bbdev_enqueue_status including padding for future

This line is long.
It is always better to split lines logically,
for instance here, before "including".

> + * enum insertion

It could be made clear that the real enum size is smaller or equal.

> + */
> +#define RTE_BBDEV_ENQ_STATUS_SIZE_MAX 6
[...]
> +enum rte_bbdev_enqueue_status {
> +	RTE_BBDEV_ENQ_STATUS_NONE,             /**< Nothing to report */
> +	RTE_BBDEV_ENQ_STATUS_QUEUE_FULL,       /**< Not enough room in queue */
> +	RTE_BBDEV_ENQ_STATUS_RING_FULL,        /**< Not enough room in ring */
> +	RTE_BBDEV_ENQ_STATUS_INVALID_OP,       /**< Operation was rejected as invalid */
> +};

A comment is missing at the end of the enum to remind updating the MAX.

But the big question is why do we need this "MAX" value?
The guideline is to avoid using such MAX value for long term compatibility.

[...]
> +/**
> + * Converts queue status from enum to string

Should be imperative form: "Convert".
A dot is missing at the end of the sentence.

> + *
> + * @param status
> + *   Queue status as enum
> + *
> + * @returns
> + *  Queue status as string or NULL if op_type is invalid

It is not aligned with above parameter.
Choose an indentation format and keep it consistent.

[...]
>  	# added in 22.11
>  	rte_bbdev_device_status_str;
> +	rte_bbdev_enqueue_status_str;
>  	rte_bbdev_enqueue_fft_ops;
>  	rte_bbdev_dequeue_fft_ops;

It is not alphabetical order.