On Fri, Jul 08, 2016 at 02:00:20PM +0000, De Lara Guarch, Pablo wrote:

quoted

-----Original Message-----
From: Neil Horman [mailto:nhorman@tuxdriver.com]
Sent: Friday, July 08, 2016 1:18 PM
To: De Lara Guarch, Pablo
Cc: dev@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:

quoted

Hi Neil,

quoted

-----Original Message-----
From: Neil Horman [mailto:nhorman@tuxdriver.com]
Sent: Thursday, July 07, 2016 4:38 PM
To: dev@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

quoted

quoted

cryptodev names being available in macros.  This patch fixes the pmd

naming

quoted

quoted

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

quoted

quoted

both a
processing token and stringified form.  As such the names are defined now

as

quoted

quoted

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

quoted

quoted

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.

quoted

quoted

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Bruce Richardson <redacted>
CC: Thomas Monjalon <redacted>
CC: "De Lara Guarch, Pablo" <redacted>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: Panu Matilainen <redacted>
---
 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

...

quoted

quoted

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.

quoted

I can do that as well, if you want (maybe a separate patch, as this one is

only related to crypto).

quoted

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.

quoted

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.

quoted

Neil

quoted

Thanks,
Pablo