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