[v1] bbdev: allow operation type enum for growth

  Updating the last enum for rte_bbdev_op_type
to allow for enum insertion.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 lib/bbdev/rte_bbdev.c    | 5 ++++-
 lib/bbdev/rte_bbdev_op.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)
  

On Mon, 13 Jun 2022 11:24:35 -0700
Nicolas Chautru <nicolas.chautru@intel.com> wrote:

> Updating the last enum for rte_bbdev_op_type
> to allow for enum insertion.
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>

Only allowed if you check now for the any of the reserved types
and fail.
  

Hi Stephen, 

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, June 13, 2022 12:48 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; maxime.coquelin@redhat.com;
> trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
> david.marchand@redhat.com
> Subject: Re: [PATCH v1] bbdev: allow operation type enum for growth
> 
> On Mon, 13 Jun 2022 11:24:35 -0700
> Nicolas Chautru <nicolas.chautru@intel.com> wrote:
> 
> > Updating the last enum for rte_bbdev_op_type to allow for enum
> > insertion.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> 
> Only allowed if you check now for the any of the reserved types and fail.

Let me try to clarify what you mean. You would enforce such check in which function? Do you mean in any implementation of bbdev function taking enum rte_bbdev_op_type as argument?
In that case do you mean having in effect 2 values: 
- one for the number of supported implemented operation type = could be kept private within rte_bbdev.c implementation, purely to reject using a non-supported operation type
- and another higher value for padded maximum numbers of operations allowing for enumeration insertion.
That would sound fine to me, but please kindly confirm this is what you are implying. 
I will also check for Thomas feedback as well, thanks.
  

This solution is what I proposed to the techboard some years ago,
but the preference was to completely remove the MAX values.


13/06/2022 20:24, Nicolas Chautru:
> Updating the last enum for rte_bbdev_op_type
> to allow for enum insertion.

Please explain that the reason is to keep ABI compatible,
and you want to keep the MAX value for array needs.

> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -1122,7 +1122,10 @@ struct rte_mempool *
>  		"RTE_BBDEV_OP_TURBO_DEC",
>  		"RTE_BBDEV_OP_TURBO_ENC",
>  		"RTE_BBDEV_OP_LDPC_DEC",
> -		"RTE_BBDEV_OP_LDPC_ENC",
> +		"RTE_BBDEV_OP_RESERVED_1",
> +		"RTE_BBDEV_OP_RESERVED_2",
> +		"RTE_BBDEV_OP_RESERVED_3",
> +		"RTE_BBDEV_OP_RESERVED_4",

As Stephen said, you should make sure that using these values
with the API functions will lead to a clear and expected error.

> @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
>  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
>  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
>  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
> -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
> +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */

You must update the comment to explain there may be a padding,
it is not exactly the count.
Maybe "MAX" is a better fit than "COUNT" in this case.
  

> From: Thomas Monjalon <thomas@monjalon.net>
> 
> This solution is what I proposed to the techboard some years ago, but the
> preference was to completely remove the MAX values.
> 

Thanks, good to see you had similar thought! I don't believe there is an actual recommendation captured in term of how to remove completely MAX values in that case below. I believe that this option is still compatible with the spirit of keeping AB future proof. 

> 
> 13/06/2022 20:24, Nicolas Chautru:
> > Updating the last enum for rte_bbdev_op_type to allow for enum
> > insertion.
> 
> Please explain that the reason is to keep ABI compatible, and you want to keep
> the MAX value for array needs.
> 
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -1122,7 +1122,10 @@ struct rte_mempool *
> >  		"RTE_BBDEV_OP_TURBO_DEC",
> >  		"RTE_BBDEV_OP_TURBO_ENC",
> >  		"RTE_BBDEV_OP_LDPC_DEC",
> > -		"RTE_BBDEV_OP_LDPC_ENC",
> > +		"RTE_BBDEV_OP_RESERVED_1",
> > +		"RTE_BBDEV_OP_RESERVED_2",
> > +		"RTE_BBDEV_OP_RESERVED_3",
> > +		"RTE_BBDEV_OP_RESERVED_4",
> 
> As Stephen said, you should make sure that using these values with the API
> functions will lead to a clear and expected error.

Yes will do this. 

> 
> > @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
> >  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
> >  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
> >  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
> > -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
> > +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */
> 
> You must update the comment to explain there may be a padding, it is not
> exactly the count.
> Maybe "MAX" is a better fit than "COUNT" in this case.
> 

OK
  

Thomas Monjalon <thomas@monjalon.net> writes:

> This solution is what I proposed to the techboard some years ago,
> but the preference was to completely remove the MAX values.


I am mindful we don't have an explicit guidance in the documentation.
I am including to add something to the abi_versioning document, that
basically says

1. Try not to use _MAX values with enumerations.
2. If you have to use _MAX values with enumeration, consider giving
yourself some headroom along with rigourous checking?

>
>
> 13/06/2022 20:24, Nicolas Chautru:
>> Updating the last enum for rte_bbdev_op_type
>> to allow for enum insertion.
>
> Please explain that the reason is to keep ABI compatible,
> and you want to keep the MAX value for array needs.
>
>> --- a/lib/bbdev/rte_bbdev.c
>> +++ b/lib/bbdev/rte_bbdev.c
>> @@ -1122,7 +1122,10 @@ struct rte_mempool *
>>  		"RTE_BBDEV_OP_TURBO_DEC",
>>  		"RTE_BBDEV_OP_TURBO_ENC",
>>  		"RTE_BBDEV_OP_LDPC_DEC",
>> -		"RTE_BBDEV_OP_LDPC_ENC",
>> +		"RTE_BBDEV_OP_RESERVED_1",
>> +		"RTE_BBDEV_OP_RESERVED_2",
>> +		"RTE_BBDEV_OP_RESERVED_3",
>> +		"RTE_BBDEV_OP_RESERVED_4",
>
> As Stephen said, you should make sure that using these values
> with the API functions will lead to a clear and expected error.
>
>> @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
>>  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
>>  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
>>  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
>> -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
>> +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */
>
> You must update the comment to explain there may be a padding,
> it is not exactly the count.
> Maybe "MAX" is a better fit than "COUNT" in this case.