[dpdk-dev] [PATCH] crypto: normalize cryptodev pmd names with macros

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Friday, July 08, 2016 1:18 PM
> To: De Lara Guarch, Pablo
> Cc: dev at dpdk.org; Richardson, Bruce; Thomas Monjalon; Stephen
> Hemminger; Panu Matilainen
> Subject: Re: [PATCH] crypto: normalize cryptodev pmd names with macros
> 
> On Fri, Jul 08, 2016 at 09:09:10AM +0000, De Lara Guarch, Pablo wrote:
> > Hi Neil,
> >
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > Sent: Thursday, July 07, 2016 4:38 PM
> > > To: dev at dpdk.org
> > > Cc: Neil Horman; Richardson, Bruce; Thomas Monjalon; De Lara Guarch,
> > > Pablo; Stephen Hemminger; Panu Matilainen
> > > Subject: [PATCH] crypto: normalize cryptodev pmd names with macros
> > >
> > > Recently reported, the introduction of pmd information exports led to a
> > > breakage of cryptodev unit tests because the test infrastructure relies on
> the
> > > cryptodev names being available in macros.  This patch fixes the pmd
> naming
> > > to
> > > use the macro names.  Note that the macro names were already pre-
> > > stringified,
> > > which won't work as the PMD_REGISTER_DRIVER macro requires the
> name in
> > > both a
> > > processing token and stringified form.  As such the names are defined now
> as
> > > tokens, and converted where needed to stringified form on demand using
> > > RTE_STR.
> > >
> > > Tested using the null and aesni_mb crypto pmds, as I don't have hardware
> for
> > > any
> > > other device.  Also not build tested on snow3g or kasumi pmd because
> > > building
> > > those requires external libraries that appear to not be open source
> licensed.
> > >
> > > Signed-off-by: Neil Horman <nhorman at tuxdriver.com>
> > > CC: Bruce Richardson <bruce.richardson at intel.com>
> > > CC: Thomas Monjalon <thomas.monjalon at 6wind.com>
> > > CC: "De Lara Guarch, Pablo" <pablo.de.lara.guarch at intel.com>
> > > CC: Stephen Hemminger <stephen at networkplumber.org>
> > > CC: Panu Matilainen <pmatilai at redhat.com>
> > > ---
> > >  app/test/test_cryptodev.c                          | 20 ++++++++++----------
> > >  app/test/test_cryptodev_perf.c                     | 18 +++++++++---------
> > >  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c           |  7 +++----
> > >  drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h   |  6 +++---
> > >  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c         |  7 +++----
> > >  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h |  2 +-
> > >  drivers/crypto/kasumi/rte_kasumi_pmd.c             |  5 ++---
> > >  drivers/crypto/null/null_crypto_pmd.c              |  7 +++----
> > >  drivers/crypto/null/null_crypto_pmd_private.h      |  6 +++---
> > >  drivers/crypto/qat/rte_qat_cryptodev.c             |  4 ++--
> > >  drivers/crypto/snow3g/rte_snow3g_pmd.c             |  4 ++--
> > >  lib/librte_cryptodev/rte_cryptodev.h               | 12 ++++++------
> > >  12 files changed, 47 insertions(+), 51 deletions(-)
> >
> > Thanks for this patch. I tested snow3g and kasumi, and they don't compile.
> > I have a fix for that, so I can send a v2 of this patch if it is OK for you.
> >
> 
> I suppose thats fine, sure, though I'm really not comfortable with an open
> source project requiring what appears to be non-open source components
> (though I
> can't really tell what the snow3g or kasumi license is).  Regardless, whats the
> compilation error?

drivers/crypto/snow3g/rte_snow3g_pmd_ops.c: In function 'snow3g_pmd_qp_create_processed_ops_ring':
drivers/crypto/snow3g/rte_snow3g_pmd_ops.c:208:152: error: 'cryptodev_snow3g_pmd' undeclared (first use in this function)
   SNOW3G_LOG_ERR("Unable to reuse existing ring %s"
                                                                                                                                                        ^     
dpdk-16.04/drivers/crypto/snow3g/rte_snow3g_pmd_ops.c:208:152: note: each undeclared identifier is reported only once for each function it appears in
dpdk-16.04/drivers/crypto/snow3g/rte_snow3g_pmd_ops.c: In function 'snow3g_pmd_session_configure':
dpdk-16.04/drivers/crypto/snow3g/rte_snow3g_pmd_ops.c:296:117: error: 'cryptodev_snow3g_pmd' undeclared (first use in this function)
   SNOW3G_LOG_ERR("invalid session struct");

...

The solution is adding RTE_STR  in SNOW3G_LOG_ERR.

...
> 
> 
> > Also, we should make these changes in the other PMDs as well, right?
> > I mean, avoid setting the name of the driver directly in the structure and go
> back to the original name.
> > I can do that as well, if you want (maybe a separate patch, as this one is
> only related to crypto).
> >
> I think thats kind of two questions:
> 
> 1) Should we remove the static setting of the name in the pmd_driver
> structure
> in favor of doing it in the registration macro?
> 
> 2) Should we be consistent in the name conversion (from the setting in the
> structure instance definition to the string in the macro parameter)?
> 
> The answer to (1) is yes, though having it in both places is harmless, since the
> former will just get overridden.  We should definately remove the static
> setting, to avoid confusion, but theres not any functional rush to do so.

Will do that in a separate patch.

> 
> The answer to (2) is yes, but I think thats already done.  I don't think we
> deviated in too many places (if any), as the strings for all the net devices
> were directly set (i.e. not through macros), and I just transferred them.

Some driver names have changed (like eth_pcap to pcap).
I can revert that to the original name and we can rename them in the next release,
after a deprecation notice, since this is breaking the API.

> 
> Neil
> 
> > Thanks,
> > Pablo
> >