diff mbox series

crypto/armv8: fix typos and compilation

Message ID 20200727085810.168970-1-ruifeng.wang@arm.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series crypto/armv8: fix typos and compilation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Ruifeng Wang July 27, 2020, 8:58 a.m. UTC 
  Typo in debug log switch macro caused debug log cannot be enabled.
Fixed the typo to match option defined in config file.

Typo in crypto dev name macro caused unexpected device name in log.
Fixed the typo to log with correct device name.

Solved compilation error when debug log is enabled:
rte_armv8_pmd.c: In function ‘process_armv8_chained_op’:
rte_armv8_pmd.c:633:22: error: expected ‘)’ before ‘crypto_func’
  ARMV8_CRYPTO_ASSERT(crypto_func != NULL);
                      ^

Fixes: 169ca3db550c ("crypto/armv8: add PMD optimized for ARMv8 processors")
Cc: stable@dpdk.org

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
https://mails.dpdk.org/archives/dev/2020-July/175241.html

 drivers/crypto/armv8/armv8_pmd_private.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

David Marchand July 27, 2020, 9:38 a.m. UTC | #1 
Hello Ruifeng,

On Mon, Jul 27, 2020 at 10:58 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
>
> Typo in debug log switch macro caused debug log cannot be enabled.
> Fixed the typo to match option defined in config file.
>
> Typo in crypto dev name macro caused unexpected device name in log.
> Fixed the typo to log with correct device name.
>
> Solved compilation error when debug log is enabled:
> rte_armv8_pmd.c: In function ‘process_armv8_chained_op’:
> rte_armv8_pmd.c:633:22: error: expected ‘)’ before ‘crypto_func’
>   ARMV8_CRYPTO_ASSERT(crypto_func != NULL);
>                       ^
>
> Fixes: 169ca3db550c ("crypto/armv8: add PMD optimized for ARMv8 processors")
> Cc: stable@dpdk.org
>

Reported-by: David Marchand <david.marchand@redhat.com>

> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> https://mails.dpdk.org/archives/dev/2020-July/175241.html
>
>  drivers/crypto/armv8/armv8_pmd_private.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/armv8/armv8_pmd_private.h b/drivers/crypto/armv8/armv8_pmd_private.h
> index e08d0df78..b183c739b 100644
> --- a/drivers/crypto/armv8/armv8_pmd_private.h
> +++ b/drivers/crypto/armv8/armv8_pmd_private.h
> @@ -12,25 +12,25 @@
>
>  #define ARMV8_CRYPTO_LOG_ERR(fmt, args...) \
>         RTE_LOG(ERR, CRYPTODEV, "[%s] %s() line %u: " fmt "\n",  \
> -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
>                         __func__, __LINE__, ## args)
>


- Those macros can use a dedicated logtype for this driver rather than
pollute the CRYPTODEV general logtype.

- Looking at their uses:
drivers/crypto/armv8/armv8_pmd_private.h:#define
ARMV8_CRYPTO_LOG_ERR(fmt, args...) \
drivers/crypto/armv8/armv8_pmd_private.h:#define
ARMV8_CRYPTO_LOG_INFO(fmt, args...) \
drivers/crypto/armv8/armv8_pmd_private.h:#define
ARMV8_CRYPTO_LOG_DBG(fmt, args...) \
drivers/crypto/armv8/armv8_pmd_private.h:#define
ARMV8_CRYPTO_LOG_INFO(fmt, args...)
drivers/crypto/armv8/armv8_pmd_private.h:#define
ARMV8_CRYPTO_LOG_DBG(fmt, args...)
drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd.c:                   ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd.c:
ARMV8_CRYPTO_LOG_ERR("Invalid/unsupported operation");
drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd.c:
ARMV8_CRYPTO_LOG_ERR("failed to create cryptodev vdev");
drivers/crypto/armv8/rte_armv8_pmd.c:   ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd_ops.c:
ARMV8_CRYPTO_LOG_INFO(
drivers/crypto/armv8/rte_armv8_pmd_ops.c:               ARMV8_CRYPTO_LOG_ERR(
drivers/crypto/armv8/rte_armv8_pmd_ops.c:
ARMV8_CRYPTO_LOG_ERR("invalid session struct");
drivers/crypto/armv8/rte_armv8_pmd_ops.c:
ARMV8_CRYPTO_LOG_ERR("failed configure session parameters");

There is nothing for debug, and the rest of these messages are in setup steps.
I would rather have them always enabled and remove the debug option entirely.

WDYT?


> -#ifdef RTE_LIBRTE_ARMV8_CRYPTO_DEBUG
> +#ifdef RTE_LIBRTE_PMD_ARMV8_CRYPTO_DEBUG
>  #define ARMV8_CRYPTO_LOG_INFO(fmt, args...) \
>         RTE_LOG(INFO, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
> -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
>                         __func__, __LINE__, ## args)
>
>  #define ARMV8_CRYPTO_LOG_DBG(fmt, args...) \
>         RTE_LOG(DEBUG, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
> -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
>                         __func__, __LINE__, ## args)
>
>  #define ARMV8_CRYPTO_ASSERT(con)                               \
>  do {                                                           \
>         if (!(con)) {                                           \
> -               rte_panic("%s(): "                              \
> -                   con "condition failed, line %u", __func__); \
> +               rte_panic("condition failed, line %u",          \
> +                       __LINE__);                              \
>         }                                                       \
>  } while (0)


RTE_VERIFY does the same.
Ruifeng Wang July 27, 2020, 10:03 a.m. UTC | #2 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 27, 2020 5:39 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: Akhil.goyal@nxp.com; dev <dev@dpdk.org>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; dpdk stable
> <stable@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] crypto/armv8: fix typos and compilation
> 
> Hello Ruifeng,
> 
> On Mon, Jul 27, 2020 at 10:58 AM Ruifeng Wang <ruifeng.wang@arm.com>
> wrote:
> >
> > Typo in debug log switch macro caused debug log cannot be enabled.
> > Fixed the typo to match option defined in config file.
> >
> > Typo in crypto dev name macro caused unexpected device name in log.
> > Fixed the typo to log with correct device name.
> >
> > Solved compilation error when debug log is enabled:
> > rte_armv8_pmd.c: In function ‘process_armv8_chained_op’:
> > rte_armv8_pmd.c:633:22: error: expected ‘)’ before ‘crypto_func’
> >   ARMV8_CRYPTO_ASSERT(crypto_func != NULL);
> >                       ^
> >
> > Fixes: 169ca3db550c ("crypto/armv8: add PMD optimized for ARMv8
> > processors")
> > Cc: stable@dpdk.org
> >
> 
> Reported-by: David Marchand <david.marchand@redhat.com>
> 
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > https://mails.dpdk.org/archives/dev/2020-July/175241.html
> >
> >  drivers/crypto/armv8/armv8_pmd_private.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/crypto/armv8/armv8_pmd_private.h
> > b/drivers/crypto/armv8/armv8_pmd_private.h
> > index e08d0df78..b183c739b 100644
> > --- a/drivers/crypto/armv8/armv8_pmd_private.h
> > +++ b/drivers/crypto/armv8/armv8_pmd_private.h
> > @@ -12,25 +12,25 @@
> >
> >  #define ARMV8_CRYPTO_LOG_ERR(fmt, args...) \
> >         RTE_LOG(ERR, CRYPTODEV, "[%s] %s() line %u: " fmt "\n",  \
> > -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> > +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
> >                         __func__, __LINE__, ## args)
> >
> 
> 
> - Those macros can use a dedicated logtype for this driver rather than pollute
> the CRYPTODEV general logtype.
> 
Agreed. Will add a dedicated logtype for this driver.

> - Looking at their uses:
> drivers/crypto/armv8/armv8_pmd_private.h:#define
> ARMV8_CRYPTO_LOG_ERR(fmt, args...) \
> drivers/crypto/armv8/armv8_pmd_private.h:#define
> ARMV8_CRYPTO_LOG_INFO(fmt, args...) \
> drivers/crypto/armv8/armv8_pmd_private.h:#define
> ARMV8_CRYPTO_LOG_DBG(fmt, args...) \
> drivers/crypto/armv8/armv8_pmd_private.h:#define
> ARMV8_CRYPTO_LOG_INFO(fmt, args...)
> drivers/crypto/armv8/armv8_pmd_private.h:#define
> ARMV8_CRYPTO_LOG_DBG(fmt, args...)
> drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd.c:
> ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd.c:
> ARMV8_CRYPTO_LOG_ERR("Invalid/unsupported operation");
> drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd.c:           ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd.c:
> ARMV8_CRYPTO_LOG_ERR("failed to create cryptodev vdev");
> drivers/crypto/armv8/rte_armv8_pmd.c:   ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd_ops.c:
> ARMV8_CRYPTO_LOG_INFO(
> drivers/crypto/armv8/rte_armv8_pmd_ops.c:
> ARMV8_CRYPTO_LOG_ERR(
> drivers/crypto/armv8/rte_armv8_pmd_ops.c:
> ARMV8_CRYPTO_LOG_ERR("invalid session struct");
> drivers/crypto/armv8/rte_armv8_pmd_ops.c:
> ARMV8_CRYPTO_LOG_ERR("failed configure session parameters");
> 
> There is nothing for debug, and the rest of these messages are in setup steps.
> I would rather have them always enabled and remove the debug option
> entirely.
> 
> WDYT?
> 
Agreed. The logs are not in data path. They can be always enabled.
Will change in next version. 

> 
> > -#ifdef RTE_LIBRTE_ARMV8_CRYPTO_DEBUG
> > +#ifdef RTE_LIBRTE_PMD_ARMV8_CRYPTO_DEBUG
> >  #define ARMV8_CRYPTO_LOG_INFO(fmt, args...) \
> >         RTE_LOG(INFO, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
> > -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> > +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
> >                         __func__, __LINE__, ## args)
> >
> >  #define ARMV8_CRYPTO_LOG_DBG(fmt, args...) \
> >         RTE_LOG(DEBUG, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
> > -                       RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
> > +                       RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
> >                         __func__, __LINE__, ## args)
> >
> >  #define ARMV8_CRYPTO_ASSERT(con)                               \
> >  do {                                                           \
> >         if (!(con)) {                                           \
> > -               rte_panic("%s(): "                              \
> > -                   con "condition failed, line %u", __func__); \
> > +               rte_panic("condition failed, line %u",          \
> > +                       __LINE__);                              \
> >         }                                                       \
> >  } while (0)
> 
> 
> RTE_VERIFY does the same.
> 
Yes. Will switch to RTE_VERIFY in next version.

> 
> --
> David Marchand
diff mbox series

Patch

diff --git a/drivers/crypto/armv8/armv8_pmd_private.h b/drivers/crypto/armv8/armv8_pmd_private.h
index e08d0df78..b183c739b 100644
--- a/drivers/crypto/armv8/armv8_pmd_private.h
+++ b/drivers/crypto/armv8/armv8_pmd_private.h
@@ -12,25 +12,25 @@ 
 
 #define ARMV8_CRYPTO_LOG_ERR(fmt, args...) \
 	RTE_LOG(ERR, CRYPTODEV, "[%s] %s() line %u: " fmt "\n",  \
-			RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
+			RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
 			__func__, __LINE__, ## args)
 
-#ifdef RTE_LIBRTE_ARMV8_CRYPTO_DEBUG
+#ifdef RTE_LIBRTE_PMD_ARMV8_CRYPTO_DEBUG
 #define ARMV8_CRYPTO_LOG_INFO(fmt, args...) \
 	RTE_LOG(INFO, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
-			RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
+			RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
 			__func__, __LINE__, ## args)
 
 #define ARMV8_CRYPTO_LOG_DBG(fmt, args...) \
 	RTE_LOG(DEBUG, CRYPTODEV, "[%s] %s() line %u: " fmt "\n", \
-			RTE_STR(CRYPTODEV_NAME_ARMV8_CRYPTO_PMD), \
+			RTE_STR(CRYPTODEV_NAME_ARMV8_PMD), \
 			__func__, __LINE__, ## args)
 
 #define ARMV8_CRYPTO_ASSERT(con)				\
 do {								\
 	if (!(con)) {						\
-		rte_panic("%s(): "				\
-		    con "condition failed, line %u", __func__);	\
+		rte_panic("condition failed, line %u",		\
+			__LINE__);				\
 	}							\
 } while (0)