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

[Neil Horman]

On Fri, Jul 08, 2016 at 02:00:20PM +0000, De Lara Guarch, Pablo wrote:
> 
> 
> > -----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.
> 
Ah,thanks.  Its odd, I used cscope to do a find and replace for all the other
instances of the RTE_STR conversion, I wonder how I missed that.
Neil

> ...
> > 
> > 
> > > 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
> > >
>