[v2,09/33] crypto/octeontx: adds symmetric capabilities

  From: Murthy NSSR <nidadavolu.murthy@caviumnetworks.com>

This patch adds the symmetric algorithms capabilities
supported by octeontx crypto hardware.

Signed-off-by: Ankur Dwivedi <ankur.dwivedi@caviumnetworks.com>
Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
Signed-off-by: Murthy NSSR <nidadavolu.murthy@caviumnetworks.com>
Signed-off-by: Nithin Dabilpuram <nithin.dabilpuram@caviumnetworks.com>
Signed-off-by: Ragothaman Jayaraman <rjayaraman@caviumnetworks.com>
Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
Signed-off-by: Tejasree Kondoj <kondoj.tejasree@caviumnetworks.com>
---
 .../crypto/octeontx/otx_cryptodev_capabilities.h   | 595 +++++++++++++++++++++
 drivers/crypto/octeontx/otx_cryptodev_ops.c        |   6 +
 2 files changed, 601 insertions(+)
 create mode 100644 drivers/crypto/octeontx/otx_cryptodev_capabilities.h
  

> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> index d25f9c1..cc0030e 100644
> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> @@ -10,9 +10,15 @@
>   #include "cpt_pmd_logs.h"
>   
>   #include "otx_cryptodev.h"
> +#include "otx_cryptodev_capabilities.h"
>   #include "otx_cryptodev_hw_access.h"
>   #include "otx_cryptodev_ops.h"
>   
> +static const struct rte_cryptodev_capabilities otx_capabilities[] = {
> +	OTX_SYM_CAPABILITIES,
> +	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
> +};
> +

better to have otx_capabilities structure defined in the otx_cryptodev_capabilities.h

I don't see any value addition of creating a macro in one file using in a separate structure in another file

which doesn't have anything new in that structure. It would also give checkpatch error.

You can directly have a capability structure without the #define.

>   /* Alarm routines */
>   
>   static void
>
  

Hi Akhil,

On 17-09-2018 17:31, Akhil Goyal wrote:
> External Email
>
>> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c 
>> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> index d25f9c1..cc0030e 100644
>> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> @@ -10,9 +10,15 @@
>>   #include "cpt_pmd_logs.h"
>>
>>   #include "otx_cryptodev.h"
>> +#include "otx_cryptodev_capabilities.h"
>>   #include "otx_cryptodev_hw_access.h"
>>   #include "otx_cryptodev_ops.h"
>>
>> +static const struct rte_cryptodev_capabilities otx_capabilities[] = {
>> +     OTX_SYM_CAPABILITIES,
>> +     RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
>> +};
>> +
>
> better to have otx_capabilities structure defined in the 
> otx_cryptodev_capabilities.h
>
> I don't see any value addition of creating a macro in one file using 
> in a separate structure in another file
>
> which doesn't have anything new in that structure. It would also give 
> checkpatch error.
>
> You can directly have a capability structure without the #define.
This was the convention followed in qat driver.

https://git.dpdk.org/dpdk/tree/drivers/crypto/qat/qat_sym_capabilities.h

I guess it was to avoid variable definition in header. May be Pablo too 
can comment on this. I'll make the change accordingly.

Thanks,
Anoob
  

Hi Fiona,

Can you please comment on this?

We are adding all capabilities of octeontx-crypto PMD as a macro in 
otx_cryptodev_capabilites.h file and then we are using it from 
otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. As 
per my understanding, this is to ensure that cryptodev_ops file remains 
simple. For other PMDs with fewer number of capabilities, the structure 
can be populated in the .c file itself without the size of the file 
coming into the picture.

But this would cause checkpatch to report error. Akhil's suggestion is 
to move the entire definition to a header and include it from the .c 
file. I believe, the QAT approach was to avoid variable definition in 
the header. What do you think would be a better approach here?

Thanks,
Anoob
On 17-09-2018 18:05, Joseph, Anoob wrote:
> Hi Akhil,
>
> On 17-09-2018 17:31, Akhil Goyal wrote:
>> External Email
>>
>>> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c 
>>> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>> index d25f9c1..cc0030e 100644
>>> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>> @@ -10,9 +10,15 @@
>>>   #include "cpt_pmd_logs.h"
>>>
>>>   #include "otx_cryptodev.h"
>>> +#include "otx_cryptodev_capabilities.h"
>>>   #include "otx_cryptodev_hw_access.h"
>>>   #include "otx_cryptodev_ops.h"
>>>
>>> +static const struct rte_cryptodev_capabilities otx_capabilities[] = {
>>> +     OTX_SYM_CAPABILITIES,
>>> +     RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
>>> +};
>>> +
>>
>> better to have otx_capabilities structure defined in the 
>> otx_cryptodev_capabilities.h
>>
>> I don't see any value addition of creating a macro in one file using 
>> in a separate structure in another file
>>
>> which doesn't have anything new in that structure. It would also give 
>> checkpatch error.
>>
>> You can directly have a capability structure without the #define.
> This was the convention followed in qat driver.
>
> https://git.dpdk.org/dpdk/tree/drivers/crypto/qat/qat_sym_capabilities.h
>
> I guess it was to avoid variable definition in header. May be Pablo 
> too can comment on this. I'll make the change accordingly.
>
> Thanks,
> Anoob
>
  

Hi Fiona,

Did you get a chance to look at this?

Thanks,
Anoob
On 24-09-2018 17:06, Joseph, Anoob wrote:
> Hi Fiona,
>
> Can you please comment on this?
>
> We are adding all capabilities of octeontx-crypto PMD as a macro in 
> otx_cryptodev_capabilites.h file and then we are using it from 
> otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. 
> As per my understanding, this is to ensure that cryptodev_ops file 
> remains simple. For other PMDs with fewer number of capabilities, the 
> structure can be populated in the .c file itself without the size of 
> the file coming into the picture.
>
> But this would cause checkpatch to report error. Akhil's suggestion is 
> to move the entire definition to a header and include it from the .c 
> file. I believe, the QAT approach was to avoid variable definition in 
> the header. What do you think would be a better approach here?
>
> Thanks,
> Anoob
> On 17-09-2018 18:05, Joseph, Anoob wrote:
>> Hi Akhil,
>>
>> On 17-09-2018 17:31, Akhil Goyal wrote:
>>> External Email
>>>
>>>> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c 
>>>> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>>> index d25f9c1..cc0030e 100644
>>>> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>>> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>>>> @@ -10,9 +10,15 @@
>>>>   #include "cpt_pmd_logs.h"
>>>>
>>>>   #include "otx_cryptodev.h"
>>>> +#include "otx_cryptodev_capabilities.h"
>>>>   #include "otx_cryptodev_hw_access.h"
>>>>   #include "otx_cryptodev_ops.h"
>>>>
>>>> +static const struct rte_cryptodev_capabilities otx_capabilities[] = {
>>>> +     OTX_SYM_CAPABILITIES,
>>>> +     RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
>>>> +};
>>>> +
>>>
>>> better to have otx_capabilities structure defined in the 
>>> otx_cryptodev_capabilities.h
>>>
>>> I don't see any value addition of creating a macro in one file using 
>>> in a separate structure in another file
>>>
>>> which doesn't have anything new in that structure. It would also 
>>> give checkpatch error.
>>>
>>> You can directly have a capability structure without the #define.
>> This was the convention followed in qat driver.
>>
>> https://git.dpdk.org/dpdk/tree/drivers/crypto/qat/qat_sym_capabilities.h
>>
>> I guess it was to avoid variable definition in header. May be Pablo 
>> too can comment on this. I'll make the change accordingly.
>>
>> Thanks,
>> Anoob
>>
>
  

24/09/2018 13:36, Joseph, Anoob:
> Hi Fiona,
> 
> Can you please comment on this?
> 
> We are adding all capabilities of octeontx-crypto PMD as a macro in 
> otx_cryptodev_capabilites.h file and then we are using it from 
> otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. As 
> per my understanding, this is to ensure that cryptodev_ops file remains 
> simple. For other PMDs with fewer number of capabilities, the structure 
> can be populated in the .c file itself without the size of the file 
> coming into the picture.
> 
> But this would cause checkpatch to report error. Akhil's suggestion is 
> to move the entire definition to a header and include it from the .c 
> file. I believe, the QAT approach was to avoid variable definition in 
> the header. What do you think would be a better approach here?

I think we should avoid adding some code in a .h file.
And it is even worst when using macros.

I suggest defining the capabilities in a .c file.
If you don't want to bloat the main .c file, you can create a function
defined in another .c file.
  

Hi Akhil, Joseph, Thomas,
Just spotted this now.
See below.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, October 1, 2018 11:05 AM
> To: Joseph, Anoob <Anoob.Joseph@caviumnetworks.com>
> Cc: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Anoob
> Joseph <ajoseph@caviumnetworks.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Murthy NSSR <nidadavolu.murthy@caviumnetworks.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> <narayanaprasad.athreya@caviumnetworks.com>; Ankur Dwivedi
> <ankur.dwivedi@caviumnetworks.com>; Nithin Dabilpuram
> <nithin.dabilpuram@caviumnetworks.com>; Ragothaman Jayaraman
> <rjayaraman@caviumnetworks.com>; Srisivasubramanian S <ssrinivasan@caviumnetworks.com>;
> Tejasree Kondoj <kondoj.tejasree@caviumnetworks.com>
> Subject: Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric capabilities
> 
> 24/09/2018 13:36, Joseph, Anoob:
> > Hi Fiona,
> >
> > Can you please comment on this?
> >
> > We are adding all capabilities of octeontx-crypto PMD as a macro in
> > otx_cryptodev_capabilites.h file and then we are using it from
> > otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. As
> > per my understanding, this is to ensure that cryptodev_ops file remains
> > simple. For other PMDs with fewer number of capabilities, the structure
> > can be populated in the .c file itself without the size of the file
> > coming into the picture.
> >
> > But this would cause checkpatch to report error. Akhil's suggestion is
> > to move the entire definition to a header and include it from the .c
> > file. I believe, the QAT approach was to avoid variable definition in
> > the header. What do you think would be a better approach here?
> 
> I think we should avoid adding some code in a .h file.
> And it is even worst when using macros.
> 
> I suggest defining the capabilities in a .c file.
> If you don't want to bloat the main .c file, you can create a function
> defined in another .c file.
> 
I can't remember all the variations we tried, but there were a few.
I think the macro works well in this case. 
What is the issue we need to solve?
  

08/10/2018 17:59, Trahe, Fiona:
> Hi Akhil, Joseph, Thomas,
> Just spotted this now.
> See below.
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 24/09/2018 13:36, Joseph, Anoob:
> > > Hi Fiona,
> > >
> > > Can you please comment on this?
> > >
> > > We are adding all capabilities of octeontx-crypto PMD as a macro in
> > > otx_cryptodev_capabilites.h file and then we are using it from
> > > otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. As
> > > per my understanding, this is to ensure that cryptodev_ops file remains
> > > simple. For other PMDs with fewer number of capabilities, the structure
> > > can be populated in the .c file itself without the size of the file
> > > coming into the picture.
> > >
> > > But this would cause checkpatch to report error. Akhil's suggestion is
> > > to move the entire definition to a header and include it from the .c
> > > file. I believe, the QAT approach was to avoid variable definition in
> > > the header. What do you think would be a better approach here?
> > 
> > I think we should avoid adding some code in a .h file.
> > And it is even worst when using macros.
> > 
> > I suggest defining the capabilities in a .c file.
> > If you don't want to bloat the main .c file, you can create a function
> > defined in another .c file.
> > 
> I can't remember all the variations we tried, but there were a few.
> I think the macro works well in this case. 
> What is the issue we need to solve?

It is a discussion about best practice.
My answer is: avoid long macros and avoid instructions in .h file.
  

Hi Fiona,

We were following the QAT approach for defining the capabilities. OCTEON 
TX crypto PMD has similar number of capabilities and QAT was the close 
model that we could follow. I can see the advantages of the macro 
approach, but that would give a checkpatch warning. Also, Thomas didn't 
really like the idea of having long macros. So we have fixed it in the 
upstream code.

I would like to understand what would be your approach when you add 
asymmetric support. We are also adding asymmetric support and would like 
to understand how you would be adding, while supporting devices with 
varying capability.

Thanks,
Anoob
On 09-10-2018 01:57, Thomas Monjalon wrote:
> External Email
>
> 08/10/2018 17:59, Trahe, Fiona:
>> Hi Akhil, Joseph, Thomas,
>> Just spotted this now.
>> See below.
>>
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>> 24/09/2018 13:36, Joseph, Anoob:
>>>> Hi Fiona,
>>>>
>>>> Can you please comment on this?
>>>>
>>>> We are adding all capabilities of octeontx-crypto PMD as a macro in
>>>> otx_cryptodev_capabilites.h file and then we are using it from
>>>> otx_cryptodev_ops.c. This is the approach followed by QAT crypto PMD. As
>>>> per my understanding, this is to ensure that cryptodev_ops file remains
>>>> simple. For other PMDs with fewer number of capabilities, the structure
>>>> can be populated in the .c file itself without the size of the file
>>>> coming into the picture.
>>>>
>>>> But this would cause checkpatch to report error. Akhil's suggestion is
>>>> to move the entire definition to a header and include it from the .c
>>>> file. I believe, the QAT approach was to avoid variable definition in
>>>> the header. What do you think would be a better approach here?
>>> I think we should avoid adding some code in a .h file.
>>> And it is even worst when using macros.
>>>
>>> I suggest defining the capabilities in a .c file.
>>> If you don't want to bloat the main .c file, you can create a function
>>> defined in another .c file.
>>>
>> I can't remember all the variations we tried, but there were a few.
>> I think the macro works well in this case.
>> What is the issue we need to solve?
> It is a discussion about best practice.
> My answer is: avoid long macros and avoid instructions in .h file.
>
>
>
  

Hi Fiona,

Reminder!!

Thanks,
Anoob

> -----Original Message-----
> From: Joseph, Anoob
> Sent: 10 October 2018 11:10
> To: Thomas Monjalon <thomas@monjalon.net>; Trahe, Fiona
> <fiona.trahe@intel.com>
> Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; Joseph, Anoob
> <Anoob.Joseph@cavium.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> <Kondoj.Tejasree@cavium.com>
> Subject: Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric
> capabilities
> 
> Hi Fiona,
> 
> We were following the QAT approach for defining the capabilities. OCTEON
> TX crypto PMD has similar number of capabilities and QAT was the close
> model that we could follow. I can see the advantages of the macro approach,
> but that would give a checkpatch warning. Also, Thomas didn't really like the
> idea of having long macros. So we have fixed it in the upstream code.
> 
> I would like to understand what would be your approach when you add
> asymmetric support. We are also adding asymmetric support and would like
> to understand how you would be adding, while supporting devices with
> varying capability.
> 
> Thanks,
> Anoob
> On 09-10-2018 01:57, Thomas Monjalon wrote:
> > External Email
> >
> > 08/10/2018 17:59, Trahe, Fiona:
> >> Hi Akhil, Joseph, Thomas,
> >> Just spotted this now.
> >> See below.
> >>
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >>> 24/09/2018 13:36, Joseph, Anoob:
> >>>> Hi Fiona,
> >>>>
> >>>> Can you please comment on this?
> >>>>
> >>>> We are adding all capabilities of octeontx-crypto PMD as a macro in
> >>>> otx_cryptodev_capabilites.h file and then we are using it from
> >>>> otx_cryptodev_ops.c. This is the approach followed by QAT crypto
> >>>> PMD. As per my understanding, this is to ensure that cryptodev_ops
> >>>> file remains simple. For other PMDs with fewer number of
> >>>> capabilities, the structure can be populated in the .c file itself
> >>>> without the size of the file coming into the picture.
> >>>>
> >>>> But this would cause checkpatch to report error. Akhil's suggestion
> >>>> is to move the entire definition to a header and include it from
> >>>> the .c file. I believe, the QAT approach was to avoid variable
> >>>> definition in the header. What do you think would be a better approach
> here?
> >>> I think we should avoid adding some code in a .h file.
> >>> And it is even worst when using macros.
> >>>
> >>> I suggest defining the capabilities in a .c file.
> >>> If you don't want to bloat the main .c file, you can create a
> >>> function defined in another .c file.
> >>>
> >> I can't remember all the variations we tried, but there were a few.
> >> I think the macro works well in this case.
> >> What is the issue we need to solve?
> > It is a discussion about best practice.
> > My answer is: avoid long macros and avoid instructions in .h file.
> >
> >
> >
  

Hi Anoob,

Sorry for the delay, I've been travelling a lot lately.
We don't have an alternative solution - will have to explore options
when we get to that stage of the asym PMD development.
I think the macro works well for this specific case, however we'll 
look for an alternative. At the moment we don't have 
bandwidth to investigate further. If you come up with a 
neat solution we'll be happy to follow it.

Fiona

> -----Original Message-----
> From: Joseph, Anoob [mailto:Anoob.Joseph@cavium.com]
> Sent: Tuesday, October 16, 2018 10:41 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Trahe, Fiona <fiona.trahe@intel.com>
> Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur <Ankur.Dwivedi@cavium.com>; Dabilpuram,
> Nithin <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj <Kondoj.Tejasree@cavium.com>
> Subject: RE: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric capabilities
> 
> Hi Fiona,
> 
> Reminder!!
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Joseph, Anoob
> > Sent: 10 October 2018 11:10
> > To: Thomas Monjalon <thomas@monjalon.net>; Trahe, Fiona
> > <fiona.trahe@intel.com>
> > Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; Joseph, Anoob
> > <Anoob.Joseph@cavium.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> > <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> > <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> > <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> > <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> > <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> > <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> > <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> > <Kondoj.Tejasree@cavium.com>
> > Subject: Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric
> > capabilities
> >
> > Hi Fiona,
> >
> > We were following the QAT approach for defining the capabilities. OCTEON
> > TX crypto PMD has similar number of capabilities and QAT was the close
> > model that we could follow. I can see the advantages of the macro approach,
> > but that would give a checkpatch warning. Also, Thomas didn't really like the
> > idea of having long macros. So we have fixed it in the upstream code.
> >
> > I would like to understand what would be your approach when you add
> > asymmetric support. We are also adding asymmetric support and would like
> > to understand how you would be adding, while supporting devices with
> > varying capability.
> >
> > Thanks,
> > Anoob
> > On 09-10-2018 01:57, Thomas Monjalon wrote:
> > > External Email
> > >
> > > 08/10/2018 17:59, Trahe, Fiona:
> > >> Hi Akhil, Joseph, Thomas,
> > >> Just spotted this now.
> > >> See below.
> > >>
> > >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >>> 24/09/2018 13:36, Joseph, Anoob:
> > >>>> Hi Fiona,
> > >>>>
> > >>>> Can you please comment on this?
> > >>>>
> > >>>> We are adding all capabilities of octeontx-crypto PMD as a macro in
> > >>>> otx_cryptodev_capabilites.h file and then we are using it from
> > >>>> otx_cryptodev_ops.c. This is the approach followed by QAT crypto
> > >>>> PMD. As per my understanding, this is to ensure that cryptodev_ops
> > >>>> file remains simple. For other PMDs with fewer number of
> > >>>> capabilities, the structure can be populated in the .c file itself
> > >>>> without the size of the file coming into the picture.
> > >>>>
> > >>>> But this would cause checkpatch to report error. Akhil's suggestion
> > >>>> is to move the entire definition to a header and include it from
> > >>>> the .c file. I believe, the QAT approach was to avoid variable
> > >>>> definition in the header. What do you think would be a better approach
> > here?
> > >>> I think we should avoid adding some code in a .h file.
> > >>> And it is even worst when using macros.
> > >>>
> > >>> I suggest defining the capabilities in a .c file.
> > >>> If you don't want to bloat the main .c file, you can create a
> > >>> function defined in another .c file.
> > >>>
> > >> I can't remember all the variations we tried, but there were a few.
> > >> I think the macro works well in this case.
> > >> What is the issue we need to solve?
> > > It is a discussion about best practice.
> > > My answer is: avoid long macros and avoid instructions in .h file.
> > >
> > >
> > >
  

Hi Fiona,

I do agree that your solution seems to be a neat way for organizing capabilities. But Akhil & Thomas were against that idea and we had to come up with one array with all capabilities. This would not scale well when we start supporting devices with varying capabilities.

If your plan is to follow the same approach for asym support, maybe we will also follow suit and submit the required patches.

@Akhil, Thomas, thoughts?

Thanks,
Anoob

> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: 20 October 2018 02:40
> To: Joseph, Anoob <Anoob.Joseph@cavium.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> <Kondoj.Tejasree@cavium.com>; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric
> capabilities
> 
> External Email
> 
> Hi Anoob,
> 
> Sorry for the delay, I've been travelling a lot lately.
> We don't have an alternative solution - will have to explore options when we
> get to that stage of the asym PMD development.
> I think the macro works well for this specific case, however we'll look for an
> alternative. At the moment we don't have bandwidth to investigate further.
> If you come up with a neat solution we'll be happy to follow it.
> 
> Fiona
> 
> > -----Original Message-----
> > From: Joseph, Anoob [mailto:Anoob.Joseph@cavium.com]
> > Sent: Tuesday, October 16, 2018 10:41 PM
> > To: Thomas Monjalon <thomas@monjalon.net>; Trahe, Fiona
> > <fiona.trahe@intel.com>
> > Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch,
> > Pablo <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> > <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> > <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> > <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> > <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> > <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> > <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> > <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> > <Kondoj.Tejasree@cavium.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds
> > symmetric capabilities
> >
> > Hi Fiona,
> >
> > Reminder!!
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Joseph, Anoob
> > > Sent: 10 October 2018 11:10
> > > To: Thomas Monjalon <thomas@monjalon.net>; Trahe, Fiona
> > > <fiona.trahe@intel.com>
> > > Cc: dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>; Joseph, Anoob
> > > <Anoob.Joseph@cavium.com>; De Lara Guarch, Pablo
> > > <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> > > <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> > > <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> > > <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> > > <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> > > <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> > > <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> > > <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> > > <Kondoj.Tejasree@cavium.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds
> > > symmetric capabilities
> > >
> > > Hi Fiona,
> > >
> > > We were following the QAT approach for defining the capabilities.
> > > OCTEON TX crypto PMD has similar number of capabilities and QAT was
> > > the close model that we could follow. I can see the advantages of
> > > the macro approach, but that would give a checkpatch warning. Also,
> > > Thomas didn't really like the idea of having long macros. So we have fixed
> it in the upstream code.
> > >
> > > I would like to understand what would be your approach when you add
> > > asymmetric support. We are also adding asymmetric support and would
> > > like to understand how you would be adding, while supporting devices
> > > with varying capability.
> > >
> > > Thanks,
> > > Anoob
> > > On 09-10-2018 01:57, Thomas Monjalon wrote:
> > > > External Email
> > > >
> > > > 08/10/2018 17:59, Trahe, Fiona:
> > > >> Hi Akhil, Joseph, Thomas,
> > > >> Just spotted this now.
> > > >> See below.
> > > >>
> > > >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >>> 24/09/2018 13:36, Joseph, Anoob:
> > > >>>> Hi Fiona,
> > > >>>>
> > > >>>> Can you please comment on this?
> > > >>>>
> > > >>>> We are adding all capabilities of octeontx-crypto PMD as a
> > > >>>> macro in otx_cryptodev_capabilites.h file and then we are using
> > > >>>> it from otx_cryptodev_ops.c. This is the approach followed by
> > > >>>> QAT crypto PMD. As per my understanding, this is to ensure that
> > > >>>> cryptodev_ops file remains simple. For other PMDs with fewer
> > > >>>> number of capabilities, the structure can be populated in the
> > > >>>> .c file itself without the size of the file coming into the picture.
> > > >>>>
> > > >>>> But this would cause checkpatch to report error. Akhil's
> > > >>>> suggestion is to move the entire definition to a header and
> > > >>>> include it from the .c file. I believe, the QAT approach was to
> > > >>>> avoid variable definition in the header. What do you think
> > > >>>> would be a better approach
> > > here?
> > > >>> I think we should avoid adding some code in a .h file.
> > > >>> And it is even worst when using macros.
> > > >>>
> > > >>> I suggest defining the capabilities in a .c file.
> > > >>> If you don't want to bloat the main .c file, you can create a
> > > >>> function defined in another .c file.
> > > >>>
> > > >> I can't remember all the variations we tried, but there were a few.
> > > >> I think the macro works well in this case.
> > > >> What is the issue we need to solve?
> > > > It is a discussion about best practice.
> > > > My answer is: avoid long macros and avoid instructions in .h file.
> > > >
> > > >
> > > >
  

22/10/2018 05:49, Joseph, Anoob:
> Hi Fiona,
> 
> I do agree that your solution seems to be a neat way for organizing capabilities. But Akhil & Thomas were against that idea and we had to come up with one array with all capabilities. This would not scale well when we start supporting devices with varying capabilities.
> 
> If your plan is to follow the same approach for asym support, maybe we will also follow suit and submit the required patches.
> 
> @Akhil, Thomas, thoughts?

Generally speaking, macros are bad.

Why cannot you just write functions?
I don't understand your issue.
  

Hi Thomas,

I had replaced macro with functions when I revised the patch. But when more devices (with varying capabilities) need to be supported, this can get complicated. Macro approach would be simpler in that case. Right now QAT has macros and we would like to stick to what is being followed in the community.

Following would be the use case for macros,

#define QAT_BASE_GEN1_SYM_CAPABILITIES, 	\
{	CAPABILITES,				\
	...					\
}

#define QAT_EXTRA_GEN2_SYM_CAPABILITIES  	\
{	CAPABILITES,				\
	...					\
}

static const struct rte_cryptodev_capabilities qat_gen1_sym_capabilities[] = {
	QAT_BASE_GEN1_SYM_CAPABILITIES,
	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
};

static const struct rte_cryptodev_capabilities qat_gen2_sym_capabilities[] = {
	QAT_BASE_GEN1_SYM_CAPABILITIES,
	QAT_EXTRA_GEN2_SYM_CAPABILITIES,
	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
};

Without the macros, we will be required to populate the array there itself and would mean repetition of GEN1 capabilities. Either approach is fine for us, but this could complicate things when we add support for ASYM. Since QAT is already doing this, is it fine to move to that approach as we add support for ASYM? Or if QAT is to follow any other scheme, I'm fine with adopting that as well. Whatever is simple and uniform would work. 

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 22 October 2018 12:22
> To: Joseph, Anoob <Anoob.Joseph@cavium.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Akhil Goyal
> <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Murthy, Nidadavolu
> <Nidadavolu.Murthy@cavium.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Dwivedi, Ankur
> <Ankur.Dwivedi@cavium.com>; Dabilpuram, Nithin
> <Nithin.Dabilpuram@cavium.com>; Jayaraman, Ragothaman
> <Ragothaman.Jayaraman@cavium.com>; Srinivasan, Srisivasubramanian
> <Srisivasubramanian.Srinivasan@cavium.com>; Tejasree, Kondoj
> <Kondoj.Tejasree@cavium.com>
> Subject: Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric
> capabilities
> 
> External Email
> 
> 22/10/2018 05:49, Joseph, Anoob:
> > Hi Fiona,
> >
> > I do agree that your solution seems to be a neat way for organizing
> capabilities. But Akhil & Thomas were against that idea and we had to come up
> with one array with all capabilities. This would not scale well when we start
> supporting devices with varying capabilities.
> >
> > If your plan is to follow the same approach for asym support, maybe we will
> also follow suit and submit the required patches.
> >
> > @Akhil, Thomas, thoughts?
> 
> Generally speaking, macros are bad.
> 
> Why cannot you just write functions?
> I don't understand your issue.
>
  

23/10/2018 10:48, Joseph, Anoob:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 22/10/2018 05:49, Joseph, Anoob:
> > > Hi Fiona,
> > >
> > > I do agree that your solution seems to be a neat way for organizing
> > capabilities. But Akhil & Thomas were against that idea and we had to come up
> > with one array with all capabilities. This would not scale well when we start
> > supporting devices with varying capabilities.
> > >
> > > If your plan is to follow the same approach for asym support, maybe we will
> > also follow suit and submit the required patches.
> > >
> > > @Akhil, Thomas, thoughts?
> > 
> > Generally speaking, macros are bad.
> > 
> > Why cannot you just write functions?
> > I don't understand your issue.
> 
> I had replaced macro with functions when I revised the patch. But when more devices (with varying capabilities) need to be supported, this can get complicated. Macro approach would be simpler in that case. Right now QAT has macros and we would like to stick to what is being followed in the community.
> 
> Following would be the use case for macros,
> 
> #define QAT_BASE_GEN1_SYM_CAPABILITIES, 	\
> {	CAPABILITES,				\
> 	...					\
> }
> 
> #define QAT_EXTRA_GEN2_SYM_CAPABILITIES  	\
> {	CAPABILITES,				\
> 	...					\
> }
> 
> static const struct rte_cryptodev_capabilities qat_gen1_sym_capabilities[] = {
> 	QAT_BASE_GEN1_SYM_CAPABILITIES,
> 	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
> };
> 
> static const struct rte_cryptodev_capabilities qat_gen2_sym_capabilities[] = {
> 	QAT_BASE_GEN1_SYM_CAPABILITIES,
> 	QAT_EXTRA_GEN2_SYM_CAPABILITIES,
> 	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
> };
> 
> Without the macros, we will be required to populate the array there itself and would mean repetition of GEN1 capabilities. Either approach is fine for us, but this could complicate things when we add support for ASYM. Since QAT is already doing this, is it fine to move to that approach as we add support for ASYM? Or if QAT is to follow any other scheme, I'm fine with adopting that as well. Whatever is simple and uniform would work.

You can use simple assignments in functions and avoid repetition.

There is a warning in checkpatch about macros.
I think we should be more strict with this warning.