[dpdk-dev] [PATCH 00/10] fix global variable multiple definitions

[dpdk-dev] [PATCH 00/10] fix global variable multiple definitions

[Ferruh Yigit]

Issue has been detected by '-fno-common' gcc flag. By default compiler
still can figure out that multiple definition are the same variable and
use same storage for all definitions but this is implementation specific
behaviour and better to fix it.

Many of the cases below it is nice to have to use 'extern' keyword but
there are some defects in 'virtio, ''dpaa2_sec' & 'test' that multiple
components share same global variable unintentionally.

Ferruh Yigit (10):
  bus/fslmc: fix global variable multiple definitions
  net/igb: fix global variable multiple definitions
  crypto/null: fix global variable multiple definitions
  crypto/octeontx: fix global variable multiple definitions
  crypto/dpaa2_sec: fix global variable multiple definitions
  crypto/virtio: fix global variable multiple definitions
  compress/octeontx: fix global variable multiple definitions
  app/testpmd: fix global variable multiple definitions
  app/test-pipeline: fix global variable multiple definitions
  test: fix global variable multiple definitions

 app/test-pipeline/config.c                    |  2 +-
 app/test-pmd/cmdline_flow.c                   | 77 +++++++++++++++----
 app/test-pmd/testpmd.c                        | 35 ---------
 app/test-pmd/testpmd.h                        | 18 +++--
 app/test/test_bitratestats.c                  |  6 +-
 app/test/test_distributor_perf.c              |  2 +-
 app/test/test_efd.c                           |  2 +-
 app/test/test_efd_perf.c                      |  6 +-
 app/test/test_hash_perf.c                     | 12 +--
 app/test/test_hash_readwrite_lf.c             |  8 +-
 app/test/test_latencystats.c                  |  6 +-
 app/test/test_member_perf.c                   | 16 ++--
 app/test/test_rcu_qsbr.c                      | 10 +--
 drivers/bus/fslmc/qbman/qbman_portal.c        |  2 +
 drivers/bus/fslmc/qbman/qbman_portal.h        |  2 +-
 drivers/common/cpt/cpt_pmd_logs.h             |  2 +-
 drivers/compress/octeontx/otx_zip.h           |  2 +-
 drivers/compress/octeontx/otx_zip_pmd.c       |  2 +
 drivers/crypto/caam_jr/caam_jr.c              |  5 +-
 drivers/crypto/caam_jr/caam_jr_hw.c           |  3 +
 drivers/crypto/caam_jr/caam_jr_uio.c          |  3 +
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c   |  5 +-
 drivers/crypto/dpaa2_sec/hw/rta.h             |  1 -
 .../dpaa2_sec/hw/rta/fifo_load_store_cmd.h    |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h  |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h    |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h     |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h    |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h    |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h    |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h   |  2 -
 .../crypto/dpaa2_sec/hw/rta/operation_cmd.h   |  2 -
 .../crypto/dpaa2_sec/hw/rta/protocol_cmd.h    |  2 -
 .../dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h     |  2 -
 drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h   |  2 -
 drivers/crypto/dpaa_sec/dpaa_sec.c            |  5 +-
 drivers/crypto/null/null_crypto_pmd.c         |  1 +
 drivers/crypto/null/null_crypto_pmd_private.h |  2 +-
 drivers/crypto/octeontx/otx_cryptodev.c       |  2 +
 drivers/crypto/octeontx/otx_cryptodev.h       |  2 +-
 drivers/crypto/virtio/virtio_pci.c            |  4 +-
 drivers/crypto/virtio/virtio_pci.h            |  6 +-
 drivers/net/e1000/e1000_ethdev.h              | 12 +--
 drivers/net/e1000/igb_flow.c                  |  7 ++
 44 files changed, 151 insertions(+), 141 deletions(-)

-- 
2.21.0

[dpdk-dev] [PATCH 01/10] bus/fslmc: fix global variable multiple definitions

[Ferruh Yigit]

'qman_version' global variable is defined in a header file which was
causing multiple definitions of the variable, fixed it by moving it to
the .c file.

Issue has been detected by '-fno-common' gcc flag.

Fixes: 293c0ca94c36 ("bus/fslmc: support memory backed portals with QBMAN 5.0")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/bus/fslmc/qbman/qbman_portal.c | 2 ++
 drivers/bus/fslmc/qbman/qbman_portal.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/fslmc/qbman/qbman_portal.c b/drivers/bus/fslmc/qbman/qbman_portal.c
index e6066ce35..12a718117 100644
--- a/drivers/bus/fslmc/qbman/qbman_portal.c
+++ b/drivers/bus/fslmc/qbman/qbman_portal.c
@@ -61,6 +61,8 @@ enum qbman_sdqcr_fc {
 #define MAX_QBMAN_PORTALS  64
 static struct qbman_swp *portal_idx_map[MAX_QBMAN_PORTALS];
 
+uint32_t qman_version;
+
 /* Internal Function declaration */
 static int
 qbman_swp_enqueue_array_mode_direct(struct qbman_swp *s,
diff --git a/drivers/bus/fslmc/qbman/qbman_portal.h b/drivers/bus/fslmc/qbman/qbman_portal.h
index e54f2661c..0e9de8a1b 100644
--- a/drivers/bus/fslmc/qbman/qbman_portal.h
+++ b/drivers/bus/fslmc/qbman/qbman_portal.h
@@ -11,7 +11,7 @@
 #include "qbman_sys.h"
 #include <fsl_qbman_portal.h>
 
-uint32_t qman_version;
+extern uint32_t qman_version;
 #define QMAN_REV_4000   0x04000000
 #define QMAN_REV_4100   0x04010000
 #define QMAN_REV_4101   0x04010001
-- 
2.21.0

Re: [dpdk-dev] [PATCH 01/10] bus/fslmc: fix global variable multiple definitions

[Sachin Saxena]

Acked-by: Sachin Saxena <sachin.saxena@nxp.com>

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, September 5, 2019 8:23 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH 01/10] bus/fslmc: fix global variable multiple definitions
> 
> 'qman_version' global variable is defined in a header file which was causing
> multiple definitions of the variable, fixed it by moving it to the .c file.
> 
> Issue has been detected by '-fno-common' gcc flag.
> 
> Fixes: 293c0ca94c36 ("bus/fslmc: support memory backed portals with
> QBMAN 5.0")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/bus/fslmc/qbman/qbman_portal.c | 2 ++
> drivers/bus/fslmc/qbman/qbman_portal.h | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/fslmc/qbman/qbman_portal.c
> b/drivers/bus/fslmc/qbman/qbman_portal.c
> index e6066ce35..12a718117 100644
> --- a/drivers/bus/fslmc/qbman/qbman_portal.c
> +++ b/drivers/bus/fslmc/qbman/qbman_portal.c
> @@ -61,6 +61,8 @@ enum qbman_sdqcr_fc {
>  #define MAX_QBMAN_PORTALS  64
>  static struct qbman_swp *portal_idx_map[MAX_QBMAN_PORTALS];
> 
> +uint32_t qman_version;
> +
>  /* Internal Function declaration */
>  static int
>  qbman_swp_enqueue_array_mode_direct(struct qbman_swp *s, diff --git
> a/drivers/bus/fslmc/qbman/qbman_portal.h
> b/drivers/bus/fslmc/qbman/qbman_portal.h
> index e54f2661c..0e9de8a1b 100644
> --- a/drivers/bus/fslmc/qbman/qbman_portal.h
> +++ b/drivers/bus/fslmc/qbman/qbman_portal.h
> @@ -11,7 +11,7 @@
>  #include "qbman_sys.h"
>  #include <fsl_qbman_portal.h>
> 
> -uint32_t qman_version;
> +extern uint32_t qman_version;
>  #define QMAN_REV_4000   0x04000000
>  #define QMAN_REV_4100   0x04010000
>  #define QMAN_REV_4101   0x04010001
> --
> 2.21.0

[dpdk-dev] [PATCH 02/10] net/igb: fix global variable multiple definitions

[Ferruh Yigit]

Filtering related global variables are defined in a header file which
was causing multiple definitions of the variables, fixed it by moving
them to the .c file.

Issue has been detected by '-fno-common' gcc flag.

Fixes: 22bb13410cb2 ("net/igb: create consistent filter")
Fixes: 424ae915baf0 ("net/e1000: move RSS to flow API")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/e1000/e1000_ethdev.h | 12 ++++++------
 drivers/net/e1000/igb_flow.c     |  7 +++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 01ff9433b..1e41ae9de 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -351,17 +351,17 @@ struct igb_flow_mem {
 };
 
 TAILQ_HEAD(igb_ntuple_filter_list, igb_ntuple_filter_ele);
-struct igb_ntuple_filter_list igb_filter_ntuple_list;
+extern struct igb_ntuple_filter_list igb_filter_ntuple_list;
 TAILQ_HEAD(igb_ethertype_filter_list, igb_ethertype_filter_ele);
-struct igb_ethertype_filter_list igb_filter_ethertype_list;
+extern struct igb_ethertype_filter_list igb_filter_ethertype_list;
 TAILQ_HEAD(igb_syn_filter_list, igb_eth_syn_filter_ele);
-struct igb_syn_filter_list igb_filter_syn_list;
+extern struct igb_syn_filter_list igb_filter_syn_list;
 TAILQ_HEAD(igb_flex_filter_list, igb_flex_filter_ele);
-struct igb_flex_filter_list igb_filter_flex_list;
+extern struct igb_flex_filter_list igb_filter_flex_list;
 TAILQ_HEAD(igb_rss_filter_list, igb_rss_conf_ele);
-struct igb_rss_filter_list igb_filter_rss_list;
+extern struct igb_rss_filter_list igb_filter_rss_list;
 TAILQ_HEAD(igb_flow_mem_list, igb_flow_mem);
-struct igb_flow_mem_list igb_flow_list;
+extern struct igb_flow_mem_list igb_flow_list;
 
 extern const struct rte_flow_ops igb_flow_ops;
 
diff --git a/drivers/net/e1000/igb_flow.c b/drivers/net/e1000/igb_flow.c
index 4e0b38fcf..31ca9bb1c 100644
--- a/drivers/net/e1000/igb_flow.c
+++ b/drivers/net/e1000/igb_flow.c
@@ -49,6 +49,13 @@
 
 #define	IGB_FLEX_RAW_NUM	12
 
+struct igb_flow_mem_list igb_flow_list;
+struct igb_ntuple_filter_list igb_filter_ntuple_list;
+struct igb_ethertype_filter_list igb_filter_ethertype_list;
+struct igb_syn_filter_list igb_filter_syn_list;
+struct igb_flex_filter_list igb_filter_flex_list;
+struct igb_rss_filter_list igb_filter_rss_list;
+
 /**
  * Please aware there's an asumption for all the parsers.
  * rte_flow_item is using big endian, rte_flow_attr and
-- 
2.21.0

[dpdk-dev] [PATCH 03/10] crypto/null: fix global variable multiple definitions

[Ferruh Yigit]

'null_logtype_driver' global variable is defined in a header file which
was causing multiple definitions of the variable, fixed it by moving it
to the .c file.

Issue has been detected by '-fno-common' gcc flag.

Fixes: 735b783d8c2b ("crypto/null: add dynamic logging")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/crypto/null/null_crypto_pmd.c         | 1 +
 drivers/crypto/null/null_crypto_pmd_private.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/null/null_crypto_pmd.c b/drivers/crypto/null/null_crypto_pmd.c
index d5e3064f2..f11636c5f 100644
--- a/drivers/crypto/null/null_crypto_pmd.c
+++ b/drivers/crypto/null/null_crypto_pmd.c
@@ -10,6 +10,7 @@
 #include "null_crypto_pmd_private.h"
 
 static uint8_t cryptodev_driver_id;
+int null_logtype_driver;
 
 /** verify and set session parameters */
 int
diff --git a/drivers/crypto/null/null_crypto_pmd_private.h b/drivers/crypto/null/null_crypto_pmd_private.h
index d7bfd9cc8..89c4345b6 100644
--- a/drivers/crypto/null/null_crypto_pmd_private.h
+++ b/drivers/crypto/null/null_crypto_pmd_private.h
@@ -8,7 +8,7 @@
 #define CRYPTODEV_NAME_NULL_PMD		crypto_null
 /**< Null crypto PMD device name */
 
-int null_logtype_driver;
+extern int null_logtype_driver;
 
 #define NULL_LOG(level, fmt, ...)  \
 	rte_log(RTE_LOG_ ## level, null_logtype_driver,  \
-- 
2.21.0

[dpdk-dev] [PATCH 04/10] crypto/octeontx: fix global variable multiple definitions

[Ferruh Yigit]

'cpt_logtype' & 'otx_cryptodev_driver_id' global variables are defined
in a header file which was causing multiple definitions of the
variables, fixed it by moving them to the .c file.

Issue has been detected by '-fno-common' gcc flag.

Fixes: bfe2ae495ee2 ("crypto/octeontx: add PMD skeleton")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/common/cpt/cpt_pmd_logs.h       | 2 +-
 drivers/crypto/octeontx/otx_cryptodev.c | 2 ++
 drivers/crypto/octeontx/otx_cryptodev.h | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/common/cpt/cpt_pmd_logs.h b/drivers/common/cpt/cpt_pmd_logs.h
index 4cbec4e36..2681d1286 100644
--- a/drivers/common/cpt/cpt_pmd_logs.h
+++ b/drivers/common/cpt/cpt_pmd_logs.h
@@ -45,6 +45,6 @@
  * cpt_logtype will be used for common logging. This field would be initialized
  * by otx_* driver routines during PCI probe.
  */
-int cpt_logtype;
+extern int cpt_logtype;
 
 #endif /* _CPT_PMD_LOGS_H_ */
diff --git a/drivers/crypto/octeontx/otx_cryptodev.c b/drivers/crypto/octeontx/otx_cryptodev.c
index fc64a5f30..604dc2cdb 100644
--- a/drivers/crypto/octeontx/otx_cryptodev.c
+++ b/drivers/crypto/octeontx/otx_cryptodev.c
@@ -16,6 +16,8 @@
 #include "otx_cryptodev_ops.h"
 
 static int otx_cryptodev_logtype;
+int cpt_logtype;
+uint8_t otx_cryptodev_driver_id;
 
 static struct rte_pci_id pci_id_cpt_table[] = {
 	{
diff --git a/drivers/crypto/octeontx/otx_cryptodev.h b/drivers/crypto/octeontx/otx_cryptodev.h
index 6c2871d71..0b204320a 100644
--- a/drivers/crypto/octeontx/otx_cryptodev.h
+++ b/drivers/crypto/octeontx/otx_cryptodev.h
@@ -15,6 +15,6 @@
 /*
  * Crypto device driver ID
  */
-uint8_t otx_cryptodev_driver_id;
+extern uint8_t otx_cryptodev_driver_id;
 
 #endif /* _OTX_CRYPTODEV_H_ */
-- 
2.21.0

Re: [dpdk-dev] [EXT] [PATCH 04/10] crypto/octeontx: fix global variable multiple definitions

[Anoob Joseph]

Hi Ferruh,

This patch could be problematic as our new PMD(crypto_octeontx2) also makes use of some of these. I will propose a new patch with the required changes so that you wouldn't see the mentioned issue. Does that sound ok?

Thanks,
Anoob

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, September 5, 2019 8:23 PM
> To: Anoob Joseph <anoobj@marvell.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [EXT] [PATCH 04/10] crypto/octeontx: fix global variable multiple
> definitions
> 
> External Email
> 
> ----------------------------------------------------------------------
> 'cpt_logtype' & 'otx_cryptodev_driver_id' global variables are defined in a
> header file which was causing multiple definitions of the variables, fixed it by
> moving them to the .c file.
> 
> Issue has been detected by '-fno-common' gcc flag.
> 
> Fixes: bfe2ae495ee2 ("crypto/octeontx: add PMD skeleton")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/common/cpt/cpt_pmd_logs.h       | 2 +-
>  drivers/crypto/octeontx/otx_cryptodev.c | 2 ++
> drivers/crypto/octeontx/otx_cryptodev.h | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/cpt/cpt_pmd_logs.h
> b/drivers/common/cpt/cpt_pmd_logs.h
> index 4cbec4e36..2681d1286 100644
> --- a/drivers/common/cpt/cpt_pmd_logs.h
> +++ b/drivers/common/cpt/cpt_pmd_logs.h
> @@ -45,6 +45,6 @@
>   * cpt_logtype will be used for common logging. This field would be initialized
>   * by otx_* driver routines during PCI probe.
>   */
> -int cpt_logtype;
> +extern int cpt_logtype;
> 
>  #endif /* _CPT_PMD_LOGS_H_ */
> diff --git a/drivers/crypto/octeontx/otx_cryptodev.c
> b/drivers/crypto/octeontx/otx_cryptodev.c
> index fc64a5f30..604dc2cdb 100644
> --- a/drivers/crypto/octeontx/otx_cryptodev.c
> +++ b/drivers/crypto/octeontx/otx_cryptodev.c
> @@ -16,6 +16,8 @@
>  #include "otx_cryptodev_ops.h"
> 
>  static int otx_cryptodev_logtype;
> +int cpt_logtype;
> +uint8_t otx_cryptodev_driver_id;
> 
>  static struct rte_pci_id pci_id_cpt_table[] = {
>  	{
> diff --git a/drivers/crypto/octeontx/otx_cryptodev.h
> b/drivers/crypto/octeontx/otx_cryptodev.h
> index 6c2871d71..0b204320a 100644
> --- a/drivers/crypto/octeontx/otx_cryptodev.h
> +++ b/drivers/crypto/octeontx/otx_cryptodev.h
> @@ -15,6 +15,6 @@
>  /*
>   * Crypto device driver ID
>   */
> -uint8_t otx_cryptodev_driver_id;
> +extern uint8_t otx_cryptodev_driver_id;
> 
>  #endif /* _OTX_CRYPTODEV_H_ */
> --
> 2.21.0

Re: [dpdk-dev] [EXT] [PATCH 04/10] crypto/octeontx: fix global variable multiple definitions

[Ferruh Yigit]

On 9/26/2019 12:20 PM, Anoob Joseph wrote:
> Hi Ferruh,
> 
> This patch could be problematic as our new PMD(crypto_octeontx2) also makes use of some of these. I will propose a new patch with the required changes so that you wouldn't see the mentioned issue. Does that sound ok?

Sure, no problem.

> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, September 5, 2019 8:23 PM
>> To: Anoob Joseph <anoobj@marvell.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: [EXT] [PATCH 04/10] crypto/octeontx: fix global variable multiple
>> definitions
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> 'cpt_logtype' & 'otx_cryptodev_driver_id' global variables are defined in a
>> header file which was causing multiple definitions of the variables, fixed it by
>> moving them to the .c file.
>>
>> Issue has been detected by '-fno-common' gcc flag.
>>
>> Fixes: bfe2ae495ee2 ("crypto/octeontx: add PMD skeleton")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  drivers/common/cpt/cpt_pmd_logs.h       | 2 +-
>>  drivers/crypto/octeontx/otx_cryptodev.c | 2 ++
>> drivers/crypto/octeontx/otx_cryptodev.h | 2 +-
>>  3 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/common/cpt/cpt_pmd_logs.h
>> b/drivers/common/cpt/cpt_pmd_logs.h
>> index 4cbec4e36..2681d1286 100644
>> --- a/drivers/common/cpt/cpt_pmd_logs.h
>> +++ b/drivers/common/cpt/cpt_pmd_logs.h
>> @@ -45,6 +45,6 @@
>>   * cpt_logtype will be used for common logging. This field would be initialized
>>   * by otx_* driver routines during PCI probe.
>>   */
>> -int cpt_logtype;
>> +extern int cpt_logtype;
>>
>>  #endif /* _CPT_PMD_LOGS_H_ */
>> diff --git a/drivers/crypto/octeontx/otx_cryptodev.c
>> b/drivers/crypto/octeontx/otx_cryptodev.c
>> index fc64a5f30..604dc2cdb 100644
>> --- a/drivers/crypto/octeontx/otx_cryptodev.c
>> +++ b/drivers/crypto/octeontx/otx_cryptodev.c
>> @@ -16,6 +16,8 @@
>>  #include "otx_cryptodev_ops.h"
>>
>>  static int otx_cryptodev_logtype;
>> +int cpt_logtype;
>> +uint8_t otx_cryptodev_driver_id;
>>
>>  static struct rte_pci_id pci_id_cpt_table[] = {
>>  	{
>> diff --git a/drivers/crypto/octeontx/otx_cryptodev.h
>> b/drivers/crypto/octeontx/otx_cryptodev.h
>> index 6c2871d71..0b204320a 100644
>> --- a/drivers/crypto/octeontx/otx_cryptodev.h
>> +++ b/drivers/crypto/octeontx/otx_cryptodev.h
>> @@ -15,6 +15,6 @@
>>  /*
>>   * Crypto device driver ID
>>   */
>> -uint8_t otx_cryptodev_driver_id;
>> +extern uint8_t otx_cryptodev_driver_id;
>>
>>  #endif /* _OTX_CRYPTODEV_H_ */
>> --
>> 2.21.0
> 

[dpdk-dev] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[Ferruh Yigit]

'rta_sec_era' global variable is defined in 'crypto/dpaa2_sec',
'crypto/caam_jr' & 'crypto/dpaa_sec' PMDs with the same name. This means
they share same storage when application linked with static DPDK
library, which is not the intention. Three differing drivers sharing
same global variable is a defect.

Variable has been used by multiple header files in static inline
functions and these header files are shared by all three PMDs, this
forces using same variable name in three PMDs.

Since the variable is used only single .c file in 'crypto/dpaa2_sec' &
'crypto/dpaa_sec' converting global variable to 'static', this requires
removing 'extern' definition from header files, and this requires moving
the global variable definition before including those headers in .c
files.

For 'crypto/caam_jr' multiple .c files exists and includes these
headers which prevent making variable static, so only one file has the
global variable and others has 'extern' keyword on .c files.

This change should let all three drivers have their own storage for the
'rta_sec_era' global variable without multiple definitions.

Fixes: 1d678de329ab ("crypto/caam_jr: add basic job ring routines")
Fixes: c3e85bdcc6e6 ("crypto/dpaa_sec: add crypto driver for NXP DPAA platform")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/crypto/caam_jr/caam_jr.c                      | 5 +++--
 drivers/crypto/caam_jr/caam_jr_hw.c                   | 3 +++
 drivers/crypto/caam_jr/caam_jr_uio.c                  | 3 +++
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c           | 5 +++--
 drivers/crypto/dpaa2_sec/hw/rta.h                     | 1 -
 drivers/crypto/dpaa2_sec/hw/rta/fifo_load_store_cmd.h | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h          | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h            | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h             | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h            | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h            | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h            | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h           | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/operation_cmd.h       | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/protocol_cmd.h        | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h  | 2 --
 drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h           | 2 --
 drivers/crypto/dpaa_sec/dpaa_sec.c                    | 5 +++--
 18 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
index 77c030347..3503f806f 100644
--- a/drivers/crypto/caam_jr/caam_jr.c
+++ b/drivers/crypto/caam_jr/caam_jr.c
@@ -17,6 +17,9 @@
 #include <rte_security_driver.h>
 #include <rte_hexdump.h>
 
+#include <hw/rta/sec_run_time_asm.h>
+enum rta_sec_era rta_sec_era;
+
 #include <caam_jr_capabilities.h>
 #include <caam_jr_config.h>
 #include <caam_jr_hw_specific.h>
@@ -34,8 +37,6 @@
 static uint8_t cryptodev_driver_id;
 int caam_jr_logtype;
 
-enum rta_sec_era rta_sec_era;
-
 /* Lists the states possible for the SEC user space driver. */
 enum sec_driver_state_e {
 	SEC_DRIVER_STATE_IDLE,		/* Driver not initialized */
diff --git a/drivers/crypto/caam_jr/caam_jr_hw.c b/drivers/crypto/caam_jr/caam_jr_hw.c
index 4a2b08995..0ea83622f 100644
--- a/drivers/crypto/caam_jr/caam_jr_hw.c
+++ b/drivers/crypto/caam_jr/caam_jr_hw.c
@@ -11,6 +11,9 @@
 #include <rte_crypto.h>
 #include <rte_security.h>
 
+#include <hw/rta/sec_run_time_asm.h>
+extern enum rta_sec_era rta_sec_era;
+
 #include <caam_jr_config.h>
 #include <caam_jr_hw_specific.h>
 #include <caam_jr_pvt.h>
diff --git a/drivers/crypto/caam_jr/caam_jr_uio.c b/drivers/crypto/caam_jr/caam_jr_uio.c
index afd75c9a6..acc1bb14c 100644
--- a/drivers/crypto/caam_jr/caam_jr_uio.c
+++ b/drivers/crypto/caam_jr/caam_jr_uio.c
@@ -18,6 +18,9 @@
 #include <rte_crypto.h>
 #include <rte_security.h>
 
+#include <hw/rta/sec_run_time_asm.h>
+extern enum rta_sec_era rta_sec_era;
+
 #include <caam_jr_config.h>
 #include <caam_jr_hw_specific.h>
 #include <caam_jr_pvt.h>
diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 26458e5d1..a1483df7f 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -28,6 +28,9 @@
 #include <fsl_dpseci.h>
 #include <fsl_mc_sys.h>
 
+#include <hw/rta/sec_run_time_asm.h>
+static enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
+
 #include "dpaa2_sec_priv.h"
 #include "dpaa2_sec_event.h"
 #include "dpaa2_sec_logs.h"
@@ -58,8 +61,6 @@ typedef uint64_t	dma_addr_t;
 #define SEC_FLC_DHR_OUTBOUND	-114
 #define SEC_FLC_DHR_INBOUND	0
 
-enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
-
 static uint8_t cryptodev_driver_id;
 
 int dpaa2_logtype_sec;
diff --git a/drivers/crypto/dpaa2_sec/hw/rta.h b/drivers/crypto/dpaa2_sec/hw/rta.h
index c4bbad0b4..8a9f87fd1 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta.h
@@ -149,7 +149,6 @@
  * - SEC HW block revision format is "v"
  * - SEC revision format is "x.y"
  */
-extern enum rta_sec_era rta_sec_era;
 
 /**
  * rta_set_sec_era - Set SEC Era HW block revision for which the RTA library
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/fifo_load_store_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/fifo_load_store_cmd.h
index 8c807aaa2..0ec21b07b 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/fifo_load_store_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/fifo_load_store_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_FIFO_LOAD_STORE_CMD_H__
 #define __RTA_FIFO_LOAD_STORE_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t fifo_load_table[][2] = {
 /*1*/	{ PKA0,        FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_PK_A0 },
 	{ PKA1,        FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_PK_A1 },
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h
index 0c7ea9387..dab69ad5c 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/header_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_HEADER_CMD_H__
 #define __RTA_HEADER_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 /* Allowed job header flags for each SEC Era. */
 static const uint32_t job_header_flags[] = {
 	DNR | TD | MTD | SHR | REO,
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h
index 546d22e98..b915e1cd9 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/jump_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_JUMP_CMD_H__
 #define __RTA_JUMP_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t jump_test_cond[][2] = {
 	{ NIFP,     JUMP_COND_NIFP },
 	{ NIP,      JUMP_COND_NIP },
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h
index 1ec21234a..f6dee13c6 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/key_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_KEY_CMD_H__
 #define __RTA_KEY_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 /* Allowed encryption flags for each SEC Era */
 static const uint32_t key_enc_flags[] = {
 	ENC,
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h
index f3b0dcfcb..6ed7d5ce5 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/load_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_LOAD_CMD_H__
 #define __RTA_LOAD_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 /* Allowed length and offset masks for each SEC Era in case DST = DCTRL */
 static const uint32_t load_len_mask_allowed[] = {
 	0x000000ee,
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h
index 5b28cbabb..6b0ea7cb8 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/math_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_MATH_CMD_H__
 #define __RTA_MATH_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t math_op1[][2] = {
 /*1*/	{ MATH0,     MATH_SRC0_REG0 },
 	{ MATH1,     MATH_SRC0_REG1 },
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h
index a7ff7c675..a5d2508ed 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/move_cmd.h
@@ -24,8 +24,6 @@
 #define __MOVEB		2
 #define __MOVEDW	3
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t move_src_table[][2] = {
 /*1*/	{ CONTEXT1, MOVE_SRC_CLASS1CTX },
 	{ CONTEXT2, MOVE_SRC_CLASS2CTX },
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h
index 94f775e2e..2f56f8955 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/nfifo_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_NFIFO_CMD_H__
 #define __RTA_NFIFO_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t nfifo_src[][2] = {
 /*1*/	{ IFIFO,       NFIFOENTRY_STYPE_DFIFO },
 	{ OFIFO,       NFIFOENTRY_STYPE_OFIFO },
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/operation_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/operation_cmd.h
index b85760e5b..7262327e7 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/operation_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/operation_cmd.h
@@ -12,8 +12,6 @@
 #pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
 #endif
 
-extern enum rta_sec_era rta_sec_era;
-
 static inline int
 __rta_alg_aai_aes(uint16_t aai)
 {
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/protocol_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/protocol_cmd.h
index cf8dfb910..1001e6ca9 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/protocol_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/protocol_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_PROTOCOL_CMD_H__
 #define __RTA_PROTOCOL_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static inline int
 __rta_ssl_proto(uint16_t protoinfo)
 {
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h
index ceb6a8719..1a22c0702 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/seq_in_out_ptr_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_SEQ_IN_OUT_PTR_CMD_H__
 #define __RTA_SEQ_IN_OUT_PTR_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 /* Allowed SEQ IN PTR flags for each SEC Era. */
 static const uint32_t seq_in_ptr_flags[] = {
 	RBS | INL | SGF | PRE | EXT | RTO,
diff --git a/drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h b/drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h
index 8b58e544d..da847118c 100644
--- a/drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h
+++ b/drivers/crypto/dpaa2_sec/hw/rta/store_cmd.h
@@ -8,8 +8,6 @@
 #ifndef __RTA_STORE_CMD_H__
 #define __RTA_STORE_CMD_H__
 
-extern enum rta_sec_era rta_sec_era;
-
 static const uint32_t store_src_table[][2] = {
 /*1*/	{ KEY1SZ,       LDST_CLASS_1_CCB | LDST_SRCDST_WORD_KEYSZ_REG },
 	{ KEY2SZ,       LDST_CLASS_2_CCB | LDST_SRCDST_WORD_KEYSZ_REG },
diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index 122c80a07..578e3cd05 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -29,6 +29,9 @@
 #include <fsl_qman.h>
 #include <of.h>
 
+#include <hw/rta/sec_run_time_asm.h>
+static enum rta_sec_era rta_sec_era;
+
 /* RTA header files */
 #include <hw/desc/common.h>
 #include <hw/desc/algo.h>
@@ -39,8 +42,6 @@
 #include <dpaa_sec.h>
 #include <dpaa_sec_log.h>
 
-enum rta_sec_era rta_sec_era;
-
 int dpaa_logtype_sec;
 
 static uint8_t cryptodev_driver_id;
-- 
2.21.0

Re: [dpdk-dev] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[Sachin Saxena]

Acked-by: Sachin Saxena <sachin.saxena@nxp.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[David Marchand]

On Thu, Sep 5, 2019 at 4:54 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> 'rta_sec_era' global variable is defined in 'crypto/dpaa2_sec',
> 'crypto/caam_jr' & 'crypto/dpaa_sec' PMDs with the same name. This means
> they share same storage when application linked with static DPDK
> library, which is not the intention. Three differing drivers sharing
> same global variable is a defect.
>
> Variable has been used by multiple header files in static inline
> functions and these header files are shared by all three PMDs, this
> forces using same variable name in three PMDs.
>
> Since the variable is used only single .c file in 'crypto/dpaa2_sec' &
> 'crypto/dpaa_sec' converting global variable to 'static', this requires
> removing 'extern' definition from header files, and this requires moving
> the global variable definition before including those headers in .c
> files.
>
> For 'crypto/caam_jr' multiple .c files exists and includes these
> headers which prevent making variable static, so only one file has the
> global variable and others has 'extern' keyword on .c files.
>
> This change should let all three drivers have their own storage for the
> 'rta_sec_era' global variable without multiple definitions.
>
> Fixes: 1d678de329ab ("crypto/caam_jr: add basic job ring routines")
> Fixes: c3e85bdcc6e6 ("crypto/dpaa_sec: add crypto driver for NXP DPAA platform")
> Cc: stable@dpdk.org

Hit a build issue, with gcc, static build:

../drivers/crypto/dpaa_sec/dpaa_sec.c:33:25: error: static declaration
of ‘rta_sec_era’ follows non-static declaration
   33 | static enum rta_sec_era rta_sec_era;
      |                         ^~~~~~~~~~~
In file included from
../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
                 from ../drivers/crypto/dpaa_sec/dpaa_sec.c:32:
../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/desc.h:21:25: note:
previous declaration of ‘rta_sec_era’ was here
   21 | extern enum rta_sec_era rta_sec_era;
      |                         ^~~~~~~~~~~

And

../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:33:25: error: static
declaration of ‘rta_sec_era’ follows non-static declaration
   33 | static enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
      |                         ^~~~~~~~~~~
In file included from ../drivers/crypto/dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
                 from ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:32:
../drivers/crypto/dpaa2_sec/hw/desc.h:21:25: note: previous
declaration of ‘rta_sec_era’ was here
   21 | extern enum rta_sec_era rta_sec_era;
      |                         ^~~~~~~~~~~
[5/86] Compiling C object
'drivers/a715181@@tmp_rte_pmd_caam_jr@sta/crypto_caam_jr_caam_jr.c.o'.



-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[Ferruh Yigit]

On 10/24/2019 3:53 PM, David Marchand wrote:
> On Thu, Sep 5, 2019 at 4:54 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> 'rta_sec_era' global variable is defined in 'crypto/dpaa2_sec',
>> 'crypto/caam_jr' & 'crypto/dpaa_sec' PMDs with the same name. This means
>> they share same storage when application linked with static DPDK
>> library, which is not the intention. Three differing drivers sharing
>> same global variable is a defect.
>>
>> Variable has been used by multiple header files in static inline
>> functions and these header files are shared by all three PMDs, this
>> forces using same variable name in three PMDs.
>>
>> Since the variable is used only single .c file in 'crypto/dpaa2_sec' &
>> 'crypto/dpaa_sec' converting global variable to 'static', this requires
>> removing 'extern' definition from header files, and this requires moving
>> the global variable definition before including those headers in .c
>> files.
>>
>> For 'crypto/caam_jr' multiple .c files exists and includes these
>> headers which prevent making variable static, so only one file has the
>> global variable and others has 'extern' keyword on .c files.
>>
>> This change should let all three drivers have their own storage for the
>> 'rta_sec_era' global variable without multiple definitions.
>>
>> Fixes: 1d678de329ab ("crypto/caam_jr: add basic job ring routines")
>> Fixes: c3e85bdcc6e6 ("crypto/dpaa_sec: add crypto driver for NXP DPAA platform")
>> Cc: stable@dpdk.org
> 
> Hit a build issue, with gcc, static build:
> 
> ../drivers/crypto/dpaa_sec/dpaa_sec.c:33:25: error: static declaration
> of ‘rta_sec_era’ follows non-static declaration
>    33 | static enum rta_sec_era rta_sec_era;
>       |                         ^~~~~~~~~~~
> In file included from
> ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
>                  from ../drivers/crypto/dpaa_sec/dpaa_sec.c:32:
> ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/desc.h:21:25: note:
> previous declaration of ‘rta_sec_era’ was here
>    21 | extern enum rta_sec_era rta_sec_era;
>       |                         ^~~~~~~~~~~
> 
> And
> 
> ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:33:25: error: static
> declaration of ‘rta_sec_era’ follows non-static declaration
>    33 | static enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
>       |                         ^~~~~~~~~~~
> In file included from ../drivers/crypto/dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
>                  from ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:32:
> ../drivers/crypto/dpaa2_sec/hw/desc.h:21:25: note: previous
> declaration of ‘rta_sec_era’ was here
>    21 | extern enum rta_sec_era rta_sec_era;
>       |                         ^~~~~~~~~~~
> [5/86] Compiling C object
> 'drivers/a715181@@tmp_rte_pmd_caam_jr@sta/crypto_caam_jr_caam_jr.c.o'.
> 
>
Would you be OK to defer the set to the next release?

Re: [dpdk-dev] [dpdk-stable] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[David Marchand]

On Thu, Oct 24, 2019 at 4:55 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 10/24/2019 3:53 PM, David Marchand wrote:
> > On Thu, Sep 5, 2019 at 4:54 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >>
> >> 'rta_sec_era' global variable is defined in 'crypto/dpaa2_sec',
> >> 'crypto/caam_jr' & 'crypto/dpaa_sec' PMDs with the same name. This means
> >> they share same storage when application linked with static DPDK
> >> library, which is not the intention. Three differing drivers sharing
> >> same global variable is a defect.
> >>
> >> Variable has been used by multiple header files in static inline
> >> functions and these header files are shared by all three PMDs, this
> >> forces using same variable name in three PMDs.
> >>
> >> Since the variable is used only single .c file in 'crypto/dpaa2_sec' &
> >> 'crypto/dpaa_sec' converting global variable to 'static', this requires
> >> removing 'extern' definition from header files, and this requires moving
> >> the global variable definition before including those headers in .c
> >> files.
> >>
> >> For 'crypto/caam_jr' multiple .c files exists and includes these
> >> headers which prevent making variable static, so only one file has the
> >> global variable and others has 'extern' keyword on .c files.
> >>
> >> This change should let all three drivers have their own storage for the
> >> 'rta_sec_era' global variable without multiple definitions.
> >>
> >> Fixes: 1d678de329ab ("crypto/caam_jr: add basic job ring routines")
> >> Fixes: c3e85bdcc6e6 ("crypto/dpaa_sec: add crypto driver for NXP DPAA platform")
> >> Cc: stable@dpdk.org
> >
> > Hit a build issue, with gcc, static build:
> >
> > ../drivers/crypto/dpaa_sec/dpaa_sec.c:33:25: error: static declaration
> > of ‘rta_sec_era’ follows non-static declaration
> >    33 | static enum rta_sec_era rta_sec_era;
> >       |                         ^~~~~~~~~~~
> > In file included from
> > ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
> >                  from ../drivers/crypto/dpaa_sec/dpaa_sec.c:32:
> > ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/desc.h:21:25: note:
> > previous declaration of ‘rta_sec_era’ was here
> >    21 | extern enum rta_sec_era rta_sec_era;
> >       |                         ^~~~~~~~~~~
> >
> > And
> >
> > ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:33:25: error: static
> > declaration of ‘rta_sec_era’ follows non-static declaration
> >    33 | static enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
> >       |                         ^~~~~~~~~~~
> > In file included from ../drivers/crypto/dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
> >                  from ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:32:
> > ../drivers/crypto/dpaa2_sec/hw/desc.h:21:25: note: previous
> > declaration of ‘rta_sec_era’ was here
> >    21 | extern enum rta_sec_era rta_sec_era;
> >       |                         ^~~~~~~~~~~
> > [5/86] Compiling C object
> > 'drivers/a715181@@tmp_rte_pmd_caam_jr@sta/crypto_caam_jr_caam_jr.c.o'.
> >
> >
> Would you be OK to defer the set to the next release?

Either that or I only take the other 7 patches that are fine (dropped
patch 4 and 9, as discussed some time ago).


-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 05/10] crypto/dpaa2_sec: fix global variable multiple definitions

[Ferruh Yigit]

On 10/24/2019 5:56 PM, David Marchand wrote:
> On Thu, Oct 24, 2019 at 4:55 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 10/24/2019 3:53 PM, David Marchand wrote:
>>> On Thu, Sep 5, 2019 at 4:54 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>
>>>> 'rta_sec_era' global variable is defined in 'crypto/dpaa2_sec',
>>>> 'crypto/caam_jr' & 'crypto/dpaa_sec' PMDs with the same name. This means
>>>> they share same storage when application linked with static DPDK
>>>> library, which is not the intention. Three differing drivers sharing
>>>> same global variable is a defect.
>>>>
>>>> Variable has been used by multiple header files in static inline
>>>> functions and these header files are shared by all three PMDs, this
>>>> forces using same variable name in three PMDs.
>>>>
>>>> Since the variable is used only single .c file in 'crypto/dpaa2_sec' &
>>>> 'crypto/dpaa_sec' converting global variable to 'static', this requires
>>>> removing 'extern' definition from header files, and this requires moving
>>>> the global variable definition before including those headers in .c
>>>> files.
>>>>
>>>> For 'crypto/caam_jr' multiple .c files exists and includes these
>>>> headers which prevent making variable static, so only one file has the
>>>> global variable and others has 'extern' keyword on .c files.
>>>>
>>>> This change should let all three drivers have their own storage for the
>>>> 'rta_sec_era' global variable without multiple definitions.
>>>>
>>>> Fixes: 1d678de329ab ("crypto/caam_jr: add basic job ring routines")
>>>> Fixes: c3e85bdcc6e6 ("crypto/dpaa_sec: add crypto driver for NXP DPAA platform")
>>>> Cc: stable@dpdk.org
>>>
>>> Hit a build issue, with gcc, static build:
>>>
>>> ../drivers/crypto/dpaa_sec/dpaa_sec.c:33:25: error: static declaration
>>> of ‘rta_sec_era’ follows non-static declaration
>>>    33 | static enum rta_sec_era rta_sec_era;
>>>       |                         ^~~~~~~~~~~
>>> In file included from
>>> ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
>>>                  from ../drivers/crypto/dpaa_sec/dpaa_sec.c:32:
>>> ../drivers/crypto/dpaa_sec/../dpaa2_sec/hw/desc.h:21:25: note:
>>> previous declaration of ‘rta_sec_era’ was here
>>>    21 | extern enum rta_sec_era rta_sec_era;
>>>       |                         ^~~~~~~~~~~
>>>
>>> And
>>>
>>> ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:33:25: error: static
>>> declaration of ‘rta_sec_era’ follows non-static declaration
>>>    33 | static enum rta_sec_era rta_sec_era = RTA_SEC_ERA_8;
>>>       |                         ^~~~~~~~~~~
>>> In file included from ../drivers/crypto/dpaa2_sec/hw/rta/sec_run_time_asm.h:10,
>>>                  from ../drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:32:
>>> ../drivers/crypto/dpaa2_sec/hw/desc.h:21:25: note: previous
>>> declaration of ‘rta_sec_era’ was here
>>>    21 | extern enum rta_sec_era rta_sec_era;
>>>       |                         ^~~~~~~~~~~
>>> [5/86] Compiling C object
>>> 'drivers/a715181@@tmp_rte_pmd_caam_jr@sta/crypto_caam_jr_caam_jr.c.o'.
>>>
>>>
>> Would you be OK to defer the set to the next release?
> 
> Either that or I only take the other 7 patches that are fine (dropped
> patch 4 and 9, as discussed some time ago).
> 

If you are OK to pick the patches in the series, +1 from my end.

[dpdk-dev] [PATCH 06/10] crypto/virtio: fix global variable multiple definitions

[Ferruh Yigit]

'virtio_hw_internal' global variable is defined in both 'crypto/virtio'
and 'net/virtio' PMDs. This means they share same storage when
application linked with static DPDK library, which is not the intention.

Fixing by adding 'crypto_' prefix to the 'crypto/virtio' driver.

Issue has been detected by '-fno-common' gcc flag.

Fixes: 25500d4b8076 ("crypto/virtio: support device init")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/crypto/virtio/virtio_pci.c | 4 ++--
 drivers/crypto/virtio/virtio_pci.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_pci.c b/drivers/crypto/virtio/virtio_pci.c
index 8137b3c5a..f490f6e28 100644
--- a/drivers/crypto/virtio/virtio_pci.c
+++ b/drivers/crypto/virtio/virtio_pci.c
@@ -31,7 +31,7 @@
 #define VIRTIO_PCI_CONFIG(hw) \
 		(((hw)->use_msix == VIRTIO_MSIX_ENABLED) ? 24 : 20)
 
-struct virtio_hw_internal virtio_hw_internal[RTE_MAX_VIRTIO_CRYPTO];
+struct virtio_hw_internal crypto_virtio_hw_internal[RTE_MAX_VIRTIO_CRYPTO];
 
 static inline int
 check_vq_phys_addr_ok(struct virtqueue *vq)
@@ -452,7 +452,7 @@ vtpci_cryptodev_init(struct rte_pci_device *dev, struct virtio_crypto_hw *hw)
 	 */
 	if (virtio_read_caps(dev, hw) == 0) {
 		VIRTIO_CRYPTO_INIT_LOG_INFO("modern virtio pci detected.");
-		virtio_hw_internal[hw->dev_id].vtpci_ops =
+		crypto_virtio_hw_internal[hw->dev_id].vtpci_ops =
 					&virtio_crypto_modern_ops;
 		hw->modern = 1;
 		return 0;
diff --git a/drivers/crypto/virtio/virtio_pci.h b/drivers/crypto/virtio/virtio_pci.h
index 604ec3662..d9a214dfd 100644
--- a/drivers/crypto/virtio/virtio_pci.h
+++ b/drivers/crypto/virtio/virtio_pci.h
@@ -201,10 +201,10 @@ struct virtio_hw_internal {
 	struct rte_pci_ioport io;
 };
 
-#define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->dev_id].vtpci_ops)
-#define VTPCI_IO(hw)	(&virtio_hw_internal[(hw)->dev_id].io)
+#define VTPCI_OPS(hw)	(crypto_virtio_hw_internal[(hw)->dev_id].vtpci_ops)
+#define VTPCI_IO(hw)	(&crypto_virtio_hw_internal[(hw)->dev_id].io)
 
-extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_VIRTIO_CRYPTO];
+extern struct virtio_hw_internal crypto_virtio_hw_internal[RTE_MAX_VIRTIO_CRYPTO];
 
 /*
  * How many bits to shift physical queue address written to QUEUE_PFN.
-- 
2.21.0

[dpdk-dev] [PATCH 07/10] compress/octeontx: fix global variable multiple definitions

[Ferruh Yigit]

'octtx_zip_logtype_driver' global variable is defined in a header file
which was causing multiple definitions of the variable, fixed it by
moving it to the .c file.

Issue has been detected by '-fno-common' gcc flag.

Fixes: 43e610bb8565 ("compress/octeontx: introduce octeontx zip PMD")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/compress/octeontx/otx_zip.h     | 2 +-
 drivers/compress/octeontx/otx_zip_pmd.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/compress/octeontx/otx_zip.h b/drivers/compress/octeontx/otx_zip.h
index 3abefd1dc..e43f7f5c3 100644
--- a/drivers/compress/octeontx/otx_zip.h
+++ b/drivers/compress/octeontx/otx_zip.h
@@ -17,7 +17,7 @@
 
 #include <zip_regs.h>
 
-int octtx_zip_logtype_driver;
+extern int octtx_zip_logtype_driver;
 
 /* ZIP VF Control/Status registers (CSRs): */
 /* VF_BAR0: */
diff --git a/drivers/compress/octeontx/otx_zip_pmd.c b/drivers/compress/octeontx/otx_zip_pmd.c
index a1651b22e..9e00c8663 100644
--- a/drivers/compress/octeontx/otx_zip_pmd.c
+++ b/drivers/compress/octeontx/otx_zip_pmd.c
@@ -11,6 +11,8 @@
 
 #include "otx_zip.h"
 
+int octtx_zip_logtype_driver;
+
 static const struct rte_compressdev_capabilities
 				octtx_zip_pmd_capabilities[] = {
 	{	.algo = RTE_COMP_ALGO_DEFLATE,
-- 
2.21.0

Re: [dpdk-dev] [EXT] [PATCH 07/10] compress/octeontx: fix global variable multiple definitions

[Ashish Gupta]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, September 5, 2019 8:53 PM
> To: Ashish Gupta <ashishg@marvell.com>; Fiona Trahe
> <fiona.trahe@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [EXT] [PATCH 07/10] compress/octeontx: fix global variable multiple
> definitions
> 
> External Email
> 
> ----------------------------------------------------------------------
> 'octtx_zip_logtype_driver' global variable is defined in a header file which
> was causing multiple definitions of the variable, fixed it by moving it to the .c
> file.
> 
> Issue has been detected by '-fno-common' gcc flag.
> 
> Fixes: 43e610bb8565 ("compress/octeontx: introduce octeontx zip PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/compress/octeontx/otx_zip.h     | 2 +-
>  drivers/compress/octeontx/otx_zip_pmd.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/compress/octeontx/otx_zip.h
> b/drivers/compress/octeontx/otx_zip.h
> index 3abefd1dc..e43f7f5c3 100644
> --- a/drivers/compress/octeontx/otx_zip.h
> +++ b/drivers/compress/octeontx/otx_zip.h
> @@ -17,7 +17,7 @@
> 
>  #include <zip_regs.h>
> 
> -int octtx_zip_logtype_driver;
> +extern int octtx_zip_logtype_driver;
> 
>  /* ZIP VF Control/Status registers (CSRs): */
>  /* VF_BAR0: */
> diff --git a/drivers/compress/octeontx/otx_zip_pmd.c
> b/drivers/compress/octeontx/otx_zip_pmd.c
> index a1651b22e..9e00c8663 100644
> --- a/drivers/compress/octeontx/otx_zip_pmd.c
> +++ b/drivers/compress/octeontx/otx_zip_pmd.c
> @@ -11,6 +11,8 @@
> 
>  #include "otx_zip.h"
> 
> +int octtx_zip_logtype_driver;
> +
>  static const struct rte_compressdev_capabilities
>  				octtx_zip_pmd_capabilities[] = {
>  	{	.algo = RTE_COMP_ALGO_DEFLATE,
> --
> 2.21.0

Acked-by: Ashish Gupta <ashishg@marvell.com>

[dpdk-dev] [PATCH 08/10] app/testpmd: fix global variable multiple definitions

[Ferruh Yigit]

Some flow config related global variables are defined in a header file
which was causing multiple definitions of the variables, fixed it by
moving them to the .c file.

Issue has been detected by '-fno-common' gcc flag.

Also while being there,
removed duplicated 'ACTION_RAW_ENCAP_MAX_DAT definition,
moved 'vxlan_encap_conf' & 'nvgre_encap_conf' initialization to
  'cmdline_flow.c' which is better location than 'testpmd.c'
relocated 'action_raw_encap_data' & 'action_raw_decap_data' struct
   definitions slightly within the file

Fixes: 1960be7d32f8 ("app/testpmd: add VXLAN encap/decap")
Fixes: dcd962fc6b4e ("app/testpmd: add NVGRE encap/decap")
Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/cmdline_flow.c | 77 +++++++++++++++++++++++++++++--------
 app/test-pmd/testpmd.c      | 35 -----------------
 app/test-pmd/testpmd.h      | 18 +++++----
 3 files changed, 71 insertions(+), 59 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 369426cbd..bc2ba7987 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -316,9 +316,7 @@ struct action_rss_data {
 	uint16_t queue[ACTION_RSS_QUEUE_NUM];
 };
 
-/** Maximum number of items in struct rte_flow_action_vxlan_encap. */
-#define ACTION_VXLAN_ENCAP_ITEMS_NUM 6
-
+/** Maximum data size in struct rte_flow_action_raw_encap. */
 #define ACTION_RAW_ENCAP_MAX_DATA 128
 
 /** Storage for struct rte_flow_action_raw_encap. */
@@ -330,6 +328,13 @@ struct raw_encap_conf {
 
 struct raw_encap_conf raw_encap_conf = {.size = 0};
 
+/** Storage for struct rte_flow_action_raw_encap including external data. */
+struct action_raw_encap_data {
+	struct rte_flow_action_raw_encap conf;
+	uint8_t data[ACTION_RAW_ENCAP_MAX_DATA];
+	uint8_t preserve[ACTION_RAW_ENCAP_MAX_DATA];
+};
+
 /** Storage for struct rte_flow_action_raw_decap. */
 struct raw_decap_conf {
 	uint8_t data[ACTION_RAW_ENCAP_MAX_DATA];
@@ -338,6 +343,35 @@ struct raw_decap_conf {
 
 struct raw_decap_conf raw_decap_conf = {.size = 0};
 
+/** Storage for struct rte_flow_action_raw_decap including external data. */
+struct action_raw_decap_data {
+	struct rte_flow_action_raw_decap conf;
+	uint8_t data[ACTION_RAW_ENCAP_MAX_DATA];
+};
+
+struct vxlan_encap_conf vxlan_encap_conf = {
+	.select_ipv4 = 1,
+	.select_vlan = 0,
+	.select_tos_ttl = 0,
+	.vni = "\x00\x00\x00",
+	.udp_src = 0,
+	.udp_dst = RTE_BE16(4789),
+	.ipv4_src = RTE_IPV4(127, 0, 0, 1),
+	.ipv4_dst = RTE_IPV4(255, 255, 255, 255),
+	.ipv6_src = "\x00\x00\x00\x00\x00\x00\x00\x00"
+		"\x00\x00\x00\x00\x00\x00\x00\x01",
+	.ipv6_dst = "\x00\x00\x00\x00\x00\x00\x00\x00"
+		"\x00\x00\x00\x00\x00\x00\x11\x11",
+	.vlan_tci = 0,
+	.ip_tos = 0,
+	.ip_ttl = 255,
+	.eth_src = "\x00\x00\x00\x00\x00\x00",
+	.eth_dst = "\xff\xff\xff\xff\xff\xff",
+};
+
+/** Maximum number of items in struct rte_flow_action_vxlan_encap. */
+#define ACTION_VXLAN_ENCAP_ITEMS_NUM 6
+
 /** Storage for struct rte_flow_action_vxlan_encap including external data. */
 struct action_vxlan_encap_data {
 	struct rte_flow_action_vxlan_encap conf;
@@ -352,6 +386,21 @@ struct action_vxlan_encap_data {
 	struct rte_flow_item_vxlan item_vxlan;
 };
 
+struct nvgre_encap_conf nvgre_encap_conf = {
+	.select_ipv4 = 1,
+	.select_vlan = 0,
+	.tni = "\x00\x00\x00",
+	.ipv4_src = RTE_IPV4(127, 0, 0, 1),
+	.ipv4_dst = RTE_IPV4(255, 255, 255, 255),
+	.ipv6_src = "\x00\x00\x00\x00\x00\x00\x00\x00"
+		"\x00\x00\x00\x00\x00\x00\x00\x01",
+	.ipv6_dst = "\x00\x00\x00\x00\x00\x00\x00\x00"
+		"\x00\x00\x00\x00\x00\x00\x11\x11",
+	.vlan_tci = 0,
+	.eth_src = "\x00\x00\x00\x00\x00\x00",
+	.eth_dst = "\xff\xff\xff\xff\xff\xff",
+};
+
 /** Maximum number of items in struct rte_flow_action_nvgre_encap. */
 #define ACTION_NVGRE_ENCAP_ITEMS_NUM 5
 
@@ -368,21 +417,17 @@ struct action_nvgre_encap_data {
 	struct rte_flow_item_nvgre item_nvgre;
 };
 
-/** Maximum data size in struct rte_flow_action_raw_encap. */
-#define ACTION_RAW_ENCAP_MAX_DATA 128
+struct l2_encap_conf l2_encap_conf;
 
-/** Storage for struct rte_flow_action_raw_encap including external data. */
-struct action_raw_encap_data {
-	struct rte_flow_action_raw_encap conf;
-	uint8_t data[ACTION_RAW_ENCAP_MAX_DATA];
-	uint8_t preserve[ACTION_RAW_ENCAP_MAX_DATA];
-};
+struct l2_decap_conf l2_decap_conf;
 
-/** Storage for struct rte_flow_action_raw_decap including external data. */
-struct action_raw_decap_data {
-	struct rte_flow_action_raw_decap conf;
-	uint8_t data[ACTION_RAW_ENCAP_MAX_DATA];
-};
+struct mplsogre_encap_conf mplsogre_encap_conf;
+
+struct mplsogre_decap_conf mplsogre_decap_conf;
+
+struct mplsoudp_encap_conf mplsoudp_encap_conf;
+
+struct mplsoudp_decap_conf mplsoudp_decap_conf;
 
 /** Maximum number of subsequent tokens and arguments on the stack. */
 #define CTX_STACK_SIZE 16
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cbf73e685..e301563d6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -478,41 +478,6 @@ uint8_t bitrate_enabled;
 struct gro_status gro_ports[RTE_MAX_ETHPORTS];
 uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES;
 
-struct vxlan_encap_conf vxlan_encap_conf = {
-	.select_ipv4 = 1,
-	.select_vlan = 0,
-	.select_tos_ttl = 0,
-	.vni = "\x00\x00\x00",
-	.udp_src = 0,
-	.udp_dst = RTE_BE16(4789),
-	.ipv4_src = RTE_IPV4(127, 0, 0, 1),
-	.ipv4_dst = RTE_IPV4(255, 255, 255, 255),
-	.ipv6_src = "\x00\x00\x00\x00\x00\x00\x00\x00"
-		"\x00\x00\x00\x00\x00\x00\x00\x01",
-	.ipv6_dst = "\x00\x00\x00\x00\x00\x00\x00\x00"
-		"\x00\x00\x00\x00\x00\x00\x11\x11",
-	.vlan_tci = 0,
-	.ip_tos = 0,
-	.ip_ttl = 255,
-	.eth_src = "\x00\x00\x00\x00\x00\x00",
-	.eth_dst = "\xff\xff\xff\xff\xff\xff",
-};
-
-struct nvgre_encap_conf nvgre_encap_conf = {
-	.select_ipv4 = 1,
-	.select_vlan = 0,
-	.tni = "\x00\x00\x00",
-	.ipv4_src = RTE_IPV4(127, 0, 0, 1),
-	.ipv4_dst = RTE_IPV4(255, 255, 255, 255),
-	.ipv6_src = "\x00\x00\x00\x00\x00\x00\x00\x00"
-		"\x00\x00\x00\x00\x00\x00\x00\x01",
-	.ipv6_dst = "\x00\x00\x00\x00\x00\x00\x00\x00"
-		"\x00\x00\x00\x00\x00\x00\x11\x11",
-	.vlan_tci = 0,
-	.eth_src = "\x00\x00\x00\x00\x00\x00",
-	.eth_dst = "\xff\xff\xff\xff\xff\xff",
-};
-
 /* Forward function declarations */
 static void setup_attached_port(portid_t pi);
 static void map_port_queue_stats_mapping_registers(portid_t pi,
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index d73955da1..79adebaeb 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -507,7 +507,8 @@ struct vxlan_encap_conf {
 	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
 	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
 };
-struct vxlan_encap_conf vxlan_encap_conf;
+
+extern struct vxlan_encap_conf vxlan_encap_conf;
 
 /* NVGRE encap/decap parameters. */
 struct nvgre_encap_conf {
@@ -522,7 +523,8 @@ struct nvgre_encap_conf {
 	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
 	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
 };
-struct nvgre_encap_conf nvgre_encap_conf;
+
+extern struct nvgre_encap_conf nvgre_encap_conf;
 
 /* L2 encap parameters. */
 struct l2_encap_conf {
@@ -532,13 +534,13 @@ struct l2_encap_conf {
 	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
 	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
 };
-struct l2_encap_conf l2_encap_conf;
+extern struct l2_encap_conf l2_encap_conf;
 
 /* L2 decap parameters. */
 struct l2_decap_conf {
 	uint32_t select_vlan:1;
 };
-struct l2_decap_conf l2_decap_conf;
+extern struct l2_decap_conf l2_decap_conf;
 
 /* MPLSoGRE encap parameters. */
 struct mplsogre_encap_conf {
@@ -553,14 +555,14 @@ struct mplsogre_encap_conf {
 	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
 	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
 };
-struct mplsogre_encap_conf mplsogre_encap_conf;
+extern struct mplsogre_encap_conf mplsogre_encap_conf;
 
 /* MPLSoGRE decap parameters. */
 struct mplsogre_decap_conf {
 	uint32_t select_ipv4:1;
 	uint32_t select_vlan:1;
 };
-struct mplsogre_decap_conf mplsogre_decap_conf;
+extern struct mplsogre_decap_conf mplsogre_decap_conf;
 
 /* MPLSoUDP encap parameters. */
 struct mplsoudp_encap_conf {
@@ -577,14 +579,14 @@ struct mplsoudp_encap_conf {
 	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
 	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
 };
-struct mplsoudp_encap_conf mplsoudp_encap_conf;
+extern struct mplsoudp_encap_conf mplsoudp_encap_conf;
 
 /* MPLSoUDP decap parameters. */
 struct mplsoudp_decap_conf {
 	uint32_t select_ipv4:1;
 	uint32_t select_vlan:1;
 };
-struct mplsoudp_decap_conf mplsoudp_decap_conf;
+extern struct mplsoudp_decap_conf mplsoudp_decap_conf;
 
 static inline unsigned int
 lcore_num(void)
-- 
2.21.0

Re: [dpdk-dev] [dpdk-stable] [PATCH 08/10] app/testpmd: fix global variable multiple definitions

[Thomas Monjalon]

05/09/2019 16:53, Ferruh Yigit:
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -507,7 +507,8 @@ struct vxlan_encap_conf {
>  	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
>  	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
>  };
> -struct vxlan_encap_conf vxlan_encap_conf;
> +
> +extern struct vxlan_encap_conf vxlan_encap_conf;
>  
>  /* NVGRE encap/decap parameters. */
>  struct nvgre_encap_conf {
> @@ -522,7 +523,8 @@ struct nvgre_encap_conf {
>  	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
>  	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
>  };
> -struct nvgre_encap_conf nvgre_encap_conf;
> +
> +extern struct nvgre_encap_conf nvgre_encap_conf;

I guess the empty line is not needed here and above.

>  /* L2 encap parameters. */
>  struct l2_encap_conf {
> @@ -532,13 +534,13 @@ struct l2_encap_conf {
>  	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
>  	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
>  };
> -struct l2_encap_conf l2_encap_conf;
> +extern struct l2_encap_conf l2_encap_conf;
>  
>  /* L2 decap parameters. */
>  struct l2_decap_conf {
>  	uint32_t select_vlan:1;
>  };
> -struct l2_decap_conf l2_decap_conf;
> +extern struct l2_decap_conf l2_decap_conf;
>  
>  /* MPLSoGRE encap parameters. */
>  struct mplsogre_encap_conf {
> @@ -553,14 +555,14 @@ struct mplsogre_encap_conf {
>  	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
>  	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
>  };
> -struct mplsogre_encap_conf mplsogre_encap_conf;
> +extern struct mplsogre_encap_conf mplsogre_encap_conf;
>  
>  /* MPLSoGRE decap parameters. */
>  struct mplsogre_decap_conf {
>  	uint32_t select_ipv4:1;
>  	uint32_t select_vlan:1;
>  };
> -struct mplsogre_decap_conf mplsogre_decap_conf;
> +extern struct mplsogre_decap_conf mplsogre_decap_conf;
>  
>  /* MPLSoUDP encap parameters. */
>  struct mplsoudp_encap_conf {
> @@ -577,14 +579,14 @@ struct mplsoudp_encap_conf {
>  	uint8_t eth_src[RTE_ETHER_ADDR_LEN];
>  	uint8_t eth_dst[RTE_ETHER_ADDR_LEN];
>  };
> -struct mplsoudp_encap_conf mplsoudp_encap_conf;
> +extern struct mplsoudp_encap_conf mplsoudp_encap_conf;
>  
>  /* MPLSoUDP decap parameters. */
>  struct mplsoudp_decap_conf {
>  	uint32_t select_ipv4:1;
>  	uint32_t select_vlan:1;
>  };
> -struct mplsoudp_decap_conf mplsoudp_decap_conf;
> +extern struct mplsoudp_decap_conf mplsoudp_decap_conf;
>  
>  static inline unsigned int
>  lcore_num(void)

[dpdk-dev] [PATCH 09/10] app/test-pipeline: fix global variable multiple definitions

[Ferruh Yigit]

'app' global variable is defined in multiple .c files, fixed it by
marking one copy as 'extern'

Issue has been detected by '-fno-common' gcc flag.

Fixes: 48f31ca50cc4 ("app/pipeline: packet framework benchmark")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pipeline/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pipeline/config.c b/app/test-pipeline/config.c
index 28ac9fcc0..42c6ed9b2 100644
--- a/app/test-pipeline/config.c
+++ b/app/test-pipeline/config.c
@@ -42,7 +42,7 @@
 
 #include "main.h"
 
-struct app_params app;
+extern struct app_params app;
 
 static const char usage[] = "\n";
 
-- 
2.21.0

Re: [dpdk-dev] [PATCH 09/10] app/test-pipeline: fix global variable multiple definitions

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, September 5, 2019 3:53 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH 09/10] app/test-pipeline: fix global variable multiple
> definitions
> 
> 'app' global variable is defined in multiple .c files, fixed it by
> marking one copy as 'extern'
> 
> Issue has been detected by '-fno-common' gcc flag.
> 
> Fixes: 48f31ca50cc4 ("app/pipeline: packet framework benchmark")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  app/test-pipeline/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-pipeline/config.c b/app/test-pipeline/config.c
> index 28ac9fcc0..42c6ed9b2 100644
> --- a/app/test-pipeline/config.c
> +++ b/app/test-pipeline/config.c
> @@ -42,7 +42,7 @@
> 
>  #include "main.h"
> 
> -struct app_params app;
> +extern struct app_params app;
> 
>  static const char usage[] = "\n";
> 
> --
> 2.21.0

The global variable "app" is already declared as extern in the main.h file, which is included into config.c file, so please remove this "app" definition in config.c altogether.

Thanks,
Cristian

Re: [dpdk-dev] [PATCH 09/10] app/test-pipeline: fix global variable multiple definitions

[Ferruh Yigit]

On 9/5/2019 4:01 PM, Dumitrescu, Cristian wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Thursday, September 5, 2019 3:53 PM
>> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: [PATCH 09/10] app/test-pipeline: fix global variable multiple
>> definitions
>>
>> 'app' global variable is defined in multiple .c files, fixed it by
>> marking one copy as 'extern'
>>
>> Issue has been detected by '-fno-common' gcc flag.
>>
>> Fixes: 48f31ca50cc4 ("app/pipeline: packet framework benchmark")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  app/test-pipeline/config.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/app/test-pipeline/config.c b/app/test-pipeline/config.c
>> index 28ac9fcc0..42c6ed9b2 100644
>> --- a/app/test-pipeline/config.c
>> +++ b/app/test-pipeline/config.c
>> @@ -42,7 +42,7 @@
>>
>>  #include "main.h"
>>
>> -struct app_params app;
>> +extern struct app_params app;
>>
>>  static const char usage[] = "\n";
>>
>> --
>> 2.21.0
> 
> The global variable "app" is already declared as extern in the main.h file, which is included into config.c file, so please remove this "app" definition in config.c altogether.

Right, I will remove in next version.

> 
> Thanks,
> Cristian
> 

[dpdk-dev] [PATCH 10/10] test: fix global variable multiple definitions

[Ferruh Yigit]

Multiple global variable are defined in multiple unit test files with
same name, but all unit test files are linked into single executable,
which means those variables share same storage which is not the
intention, fixed by making global variables 'static'.

Issue has been detected by '-fno-common' gcc flag.

Fixes: fdeb30fa7102 ("test/bitrate: add unit tests for bitrate library")
Fixes: c3eabff124e6 ("distributor: add unit tests")
Fixes: 0e925aef2779 ("app/test: add EFD functional and perf tests")
Fixes: 359e17bf081f ("app/test: improve hash unit tests")
Fixes: c7eb0972e74b ("test/hash: add lock-free r/w concurrency")
Fixes: 1e3676a06e4c ("test/latency: add unit tests for latencystats library")
Fixes: 0cc67a96e486 ("test/member: add functional and perf tests")
Fixes: e6a14121f4ae ("test/rcu: remove arbitrary limit on max core count")
Fixes: 104dbec2081a ("test/rcu: increase size of core numbers")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test/test_bitratestats.c      |  6 +++---
 app/test/test_distributor_perf.c  |  2 +-
 app/test/test_efd.c               |  2 +-
 app/test/test_efd_perf.c          |  6 +++---
 app/test/test_hash_perf.c         | 12 ++++++------
 app/test/test_hash_readwrite_lf.c |  8 ++++----
 app/test/test_latencystats.c      |  6 +++---
 app/test/test_member_perf.c       | 16 ++++++++--------
 app/test/test_rcu_qsbr.c          | 10 +++++-----
 9 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c
index 32b1b0fc0..3a7d9c037 100644
--- a/app/test/test_bitratestats.c
+++ b/app/test/test_bitratestats.c
@@ -18,9 +18,9 @@
 #define BIT_NUM_PACKETS 10
 #define QUEUE_ID 0
 
-uint16_t portid;
-struct rte_stats_bitrates *bitrate_data;
-struct rte_ring *ring;
+static uint16_t portid;
+static struct rte_stats_bitrates *bitrate_data;
+static struct rte_ring *ring;
 
 /* To test whether rte_stats_bitrate_create is successful */
 static int
diff --git a/app/test/test_distributor_perf.c b/app/test/test_distributor_perf.c
index 664530ff9..f153bcf9b 100644
--- a/app/test/test_distributor_perf.c
+++ b/app/test/test_distributor_perf.c
@@ -25,7 +25,7 @@ static volatile unsigned worker_idx;
 struct worker_stats {
 	volatile unsigned handled_packets;
 } __rte_cache_aligned;
-struct worker_stats worker_stats[RTE_MAX_LCORE];
+static struct worker_stats worker_stats[RTE_MAX_LCORE];
 
 /*
  * worker thread used for testing the time to do a round-trip of a cache
diff --git a/app/test/test_efd.c b/app/test/test_efd.c
index 73b304431..a779a71f2 100644
--- a/app/test/test_efd.c
+++ b/app/test/test_efd.c
@@ -94,7 +94,7 @@ static struct flow_key keys[5] = {
 	}
 };
 /* Array to store the data */
-efd_value_t data[5];
+static efd_value_t data[5];
 
 static inline uint8_t efd_get_all_sockets_bitmask(void)
 {
diff --git a/app/test/test_efd_perf.c b/app/test/test_efd_perf.c
index 1dcb992c6..d47622d5c 100644
--- a/app/test/test_efd_perf.c
+++ b/app/test/test_efd_perf.c
@@ -71,13 +71,13 @@ static uint32_t hashtest_key_lens[] = {
 };
 
 /* Array to store number of cycles per operation */
-uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS];
+static uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS];
 
 /* Array to store the data */
-efd_value_t data[KEYS_TO_ADD];
+static efd_value_t data[KEYS_TO_ADD];
 
 /* Array to store all input keys */
-uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
+static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
 
 /* Shuffle the keys that have been added, so lookups will be totally random */
 static void
diff --git a/app/test/test_hash_perf.c b/app/test/test_hash_perf.c
index 5648fce02..a438eae6c 100644
--- a/app/test/test_hash_perf.c
+++ b/app/test/test_hash_perf.c
@@ -53,22 +53,22 @@ static uint32_t hashtest_key_lens[] = {
 struct rte_hash *h[NUM_KEYSIZES];
 
 /* Array that stores if a slot is full */
-uint8_t slot_taken[MAX_ENTRIES];
+static uint8_t slot_taken[MAX_ENTRIES];
 
 /* Array to store number of cycles per operation */
-uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS][2][2];
+static uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS][2][2];
 
 /* Array to store all input keys */
-uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
+static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
 
 /* Array to store the precomputed hash for 'keys' */
-hash_sig_t signatures[KEYS_TO_ADD];
+static hash_sig_t signatures[KEYS_TO_ADD];
 
 /* Array to store how many busy entries have each bucket */
-uint8_t buckets[NUM_BUCKETS];
+static uint8_t buckets[NUM_BUCKETS];
 
 /* Array to store the positions where keys are added */
-int32_t positions[KEYS_TO_ADD];
+static int32_t positions[KEYS_TO_ADD];
 
 /* Parameters used for hash table in unit test functions. */
 static struct rte_hash_parameters ut_params = {
diff --git a/app/test/test_hash_readwrite_lf.c b/app/test/test_hash_readwrite_lf.c
index 1f2fba41f..97c304054 100644
--- a/app/test/test_hash_readwrite_lf.c
+++ b/app/test/test_hash_readwrite_lf.c
@@ -48,7 +48,7 @@
 #define WRITE_EXT_BKT 2
 
 #define NUM_TEST 3
-unsigned int rwc_core_cnt[NUM_TEST] = {1, 2, 4};
+static unsigned int rwc_core_cnt[NUM_TEST] = {1, 2, 4};
 
 struct rwc_perf {
 	uint32_t w_no_ks_r_hit[2][NUM_TEST];
@@ -62,7 +62,7 @@ struct rwc_perf {
 
 static struct rwc_perf rwc_lf_results, rwc_non_lf_results;
 
-struct {
+static struct {
 	uint32_t *keys;
 	uint32_t *keys_no_ks;
 	uint32_t *keys_ks;
@@ -87,9 +87,9 @@ static rte_atomic64_t greads;
 
 static volatile uint8_t writer_done;
 
-uint16_t enabled_core_ids[RTE_MAX_LCORE];
+static uint16_t enabled_core_ids[RTE_MAX_LCORE];
 
-uint8_t *scanned_bkts;
+static uint8_t *scanned_bkts;
 
 static inline uint16_t
 get_short_sig(const hash_sig_t hash)
diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
index 8dd794be4..968e0bc47 100644
--- a/app/test/test_latencystats.c
+++ b/app/test/test_latencystats.c
@@ -17,10 +17,10 @@
 #define LATENCY_NUM_PACKETS 10
 #define QUEUE_ID 0
 
-uint16_t portid;
-struct rte_ring *ring;
+static uint16_t portid;
+static struct rte_ring *ring;
 
-struct rte_metric_name lat_stats_strings[] = {
+static struct rte_metric_name lat_stats_strings[] = {
 	{"min_latency_ns"},
 	{"avg_latency_ns"},
 	{"max_latency_ns"},
diff --git a/app/test/test_member_perf.c b/app/test/test_member_perf.c
index 564a2b3c1..e2840f12d 100644
--- a/app/test/test_member_perf.c
+++ b/app/test/test_member_perf.c
@@ -65,18 +65,18 @@ static uint32_t hashtest_key_lens[] = {
 };
 
 /* Array to store number of cycles per operation */
-uint64_t cycles[NUM_TYPE][NUM_KEYSIZES][NUM_OPERATIONS];
-uint64_t false_data[NUM_TYPE][NUM_KEYSIZES];
-uint64_t false_data_bulk[NUM_TYPE][NUM_KEYSIZES];
-uint64_t false_data_multi[NUM_TYPE][NUM_KEYSIZES];
-uint64_t false_data_multi_bulk[NUM_TYPE][NUM_KEYSIZES];
+static uint64_t cycles[NUM_TYPE][NUM_KEYSIZES][NUM_OPERATIONS];
+static uint64_t false_data[NUM_TYPE][NUM_KEYSIZES];
+static uint64_t false_data_bulk[NUM_TYPE][NUM_KEYSIZES];
+static uint64_t false_data_multi[NUM_TYPE][NUM_KEYSIZES];
+static uint64_t false_data_multi_bulk[NUM_TYPE][NUM_KEYSIZES];
 
-uint64_t false_hit[NUM_TYPE][NUM_KEYSIZES];
+static uint64_t false_hit[NUM_TYPE][NUM_KEYSIZES];
 
-member_set_t data[NUM_TYPE][/* Array to store the data */KEYS_TO_ADD];
+static member_set_t data[NUM_TYPE][/* Array to store the data */KEYS_TO_ADD];
 
 /* Array to store all input keys */
-uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
+static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
 
 /* Shuffle the keys that have been added, so lookups will be totally random */
 static void
diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index d1b9e46a2..53c46ccc1 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -25,8 +25,8 @@
 /* Make sure that this has the same value as __RTE_QSBR_CNT_INIT */
 #define TEST_RCU_QSBR_CNT_INIT 1
 
-uint16_t enabled_core_ids[RTE_MAX_LCORE];
-unsigned int num_cores;
+static uint16_t enabled_core_ids[RTE_MAX_LCORE];
+static unsigned int num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -35,8 +35,8 @@ static uint32_t *hash_data[RTE_MAX_LCORE][TOTAL_ENTRY];
 static uint8_t writer_done;
 
 static struct rte_rcu_qsbr *t[RTE_MAX_LCORE];
-struct rte_hash *h[RTE_MAX_LCORE];
-char hash_name[RTE_MAX_LCORE][8];
+static struct rte_hash *h[RTE_MAX_LCORE];
+static char hash_name[RTE_MAX_LCORE][8];
 
 struct test_rcu_thread_info {
 	/* Index in RCU array */
@@ -46,7 +46,7 @@ struct test_rcu_thread_info {
 	/* lcore IDs registered on the RCU variable */
 	uint16_t r_core_ids[2];
 };
-struct test_rcu_thread_info thread_info[RTE_MAX_LCORE/4];
+static struct test_rcu_thread_info thread_info[RTE_MAX_LCORE/4];
 
 static int
 alloc_rcu(void)
-- 
2.21.0

Re: [dpdk-dev] [PATCH 10/10] test: fix global variable multiple definitions

[Honnappa Nagarahalli]

<snip>

> 
> Multiple global variable are defined in multiple unit test files with same
> name, but all unit test files are linked into single executable, which means
> those variables share same storage which is not the intention, fixed by
> making global variables 'static'.
> 
> Issue has been detected by '-fno-common' gcc flag.
> 
> Fixes: fdeb30fa7102 ("test/bitrate: add unit tests for bitrate library")
> Fixes: c3eabff124e6 ("distributor: add unit tests")
> Fixes: 0e925aef2779 ("app/test: add EFD functional and perf tests")
> Fixes: 359e17bf081f ("app/test: improve hash unit tests")
> Fixes: c7eb0972e74b ("test/hash: add lock-free r/w concurrency")
> Fixes: 1e3676a06e4c ("test/latency: add unit tests for latencystats library")
> Fixes: 0cc67a96e486 ("test/member: add functional and perf tests")
> Fixes: e6a14121f4ae ("test/rcu: remove arbitrary limit on max core count")
> Fixes: 104dbec2081a ("test/rcu: increase size of core numbers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  app/test/test_bitratestats.c      |  6 +++---
>  app/test/test_distributor_perf.c  |  2 +-
>  app/test/test_efd.c               |  2 +-
>  app/test/test_efd_perf.c          |  6 +++---
>  app/test/test_hash_perf.c         | 12 ++++++------
>  app/test/test_hash_readwrite_lf.c |  8 ++++----
>  app/test/test_latencystats.c      |  6 +++---
>  app/test/test_member_perf.c       | 16 ++++++++--------
>  app/test/test_rcu_qsbr.c          | 10 +++++-----
>  9 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c index
> 32b1b0fc0..3a7d9c037 100644
> --- a/app/test/test_bitratestats.c
> +++ b/app/test/test_bitratestats.c
> @@ -18,9 +18,9 @@
>  #define BIT_NUM_PACKETS 10
>  #define QUEUE_ID 0
> 
> -uint16_t portid;
> -struct rte_stats_bitrates *bitrate_data; -struct rte_ring *ring;
> +static uint16_t portid;
> +static struct rte_stats_bitrates *bitrate_data; static struct rte_ring
> +*ring;
> 
>  /* To test whether rte_stats_bitrate_create is successful */  static int diff --
> git a/app/test/test_distributor_perf.c b/app/test/test_distributor_perf.c
> index 664530ff9..f153bcf9b 100644
> --- a/app/test/test_distributor_perf.c
> +++ b/app/test/test_distributor_perf.c
> @@ -25,7 +25,7 @@ static volatile unsigned worker_idx;  struct
> worker_stats {
>  	volatile unsigned handled_packets;
>  } __rte_cache_aligned;
> -struct worker_stats worker_stats[RTE_MAX_LCORE];
> +static struct worker_stats worker_stats[RTE_MAX_LCORE];
> 
>  /*
>   * worker thread used for testing the time to do a round-trip of a cache diff -
> -git a/app/test/test_efd.c b/app/test/test_efd.c index 73b304431..a779a71f2
> 100644
> --- a/app/test/test_efd.c
> +++ b/app/test/test_efd.c
> @@ -94,7 +94,7 @@ static struct flow_key keys[5] = {
>  	}
>  };
>  /* Array to store the data */
> -efd_value_t data[5];
> +static efd_value_t data[5];
> 
>  static inline uint8_t efd_get_all_sockets_bitmask(void)  { diff --git
> a/app/test/test_efd_perf.c b/app/test/test_efd_perf.c index
> 1dcb992c6..d47622d5c 100644
> --- a/app/test/test_efd_perf.c
> +++ b/app/test/test_efd_perf.c
> @@ -71,13 +71,13 @@ static uint32_t hashtest_key_lens[] = {  };
> 
>  /* Array to store number of cycles per operation */ -uint64_t
> cycles[NUM_KEYSIZES][NUM_OPERATIONS];
> +static uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS];
> 
>  /* Array to store the data */
> -efd_value_t data[KEYS_TO_ADD];
> +static efd_value_t data[KEYS_TO_ADD];
> 
>  /* Array to store all input keys */
> -uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> +static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> 
>  /* Shuffle the keys that have been added, so lookups will be totally random
> */  static void diff --git a/app/test/test_hash_perf.c
> b/app/test/test_hash_perf.c index 5648fce02..a438eae6c 100644
> --- a/app/test/test_hash_perf.c
> +++ b/app/test/test_hash_perf.c
> @@ -53,22 +53,22 @@ static uint32_t hashtest_key_lens[] = {  struct
> rte_hash *h[NUM_KEYSIZES];
> 
>  /* Array that stores if a slot is full */ -uint8_t slot_taken[MAX_ENTRIES];
> +static uint8_t slot_taken[MAX_ENTRIES];
> 
>  /* Array to store number of cycles per operation */ -uint64_t
> cycles[NUM_KEYSIZES][NUM_OPERATIONS][2][2];
> +static uint64_t cycles[NUM_KEYSIZES][NUM_OPERATIONS][2][2];
> 
>  /* Array to store all input keys */
> -uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> +static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> 
>  /* Array to store the precomputed hash for 'keys' */ -hash_sig_t
> signatures[KEYS_TO_ADD];
> +static hash_sig_t signatures[KEYS_TO_ADD];
> 
>  /* Array to store how many busy entries have each bucket */ -uint8_t
> buckets[NUM_BUCKETS];
> +static uint8_t buckets[NUM_BUCKETS];
> 
>  /* Array to store the positions where keys are added */ -int32_t
> positions[KEYS_TO_ADD];
> +static int32_t positions[KEYS_TO_ADD];
> 
>  /* Parameters used for hash table in unit test functions. */  static struct
> rte_hash_parameters ut_params = { diff --git
> a/app/test/test_hash_readwrite_lf.c b/app/test/test_hash_readwrite_lf.c
> index 1f2fba41f..97c304054 100644
> --- a/app/test/test_hash_readwrite_lf.c
> +++ b/app/test/test_hash_readwrite_lf.c
> @@ -48,7 +48,7 @@
>  #define WRITE_EXT_BKT 2
> 
>  #define NUM_TEST 3
> -unsigned int rwc_core_cnt[NUM_TEST] = {1, 2, 4};
> +static unsigned int rwc_core_cnt[NUM_TEST] = {1, 2, 4};
> 
>  struct rwc_perf {
>  	uint32_t w_no_ks_r_hit[2][NUM_TEST];
> @@ -62,7 +62,7 @@ struct rwc_perf {
> 
>  static struct rwc_perf rwc_lf_results, rwc_non_lf_results;
> 
> -struct {
> +static struct {
>  	uint32_t *keys;
>  	uint32_t *keys_no_ks;
>  	uint32_t *keys_ks;
> @@ -87,9 +87,9 @@ static rte_atomic64_t greads;
> 
>  static volatile uint8_t writer_done;
> 
> -uint16_t enabled_core_ids[RTE_MAX_LCORE];
> +static uint16_t enabled_core_ids[RTE_MAX_LCORE];
> 
> -uint8_t *scanned_bkts;
> +static uint8_t *scanned_bkts;
> 
>  static inline uint16_t
>  get_short_sig(const hash_sig_t hash)
> diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c index
> 8dd794be4..968e0bc47 100644
> --- a/app/test/test_latencystats.c
> +++ b/app/test/test_latencystats.c
> @@ -17,10 +17,10 @@
>  #define LATENCY_NUM_PACKETS 10
>  #define QUEUE_ID 0
> 
> -uint16_t portid;
> -struct rte_ring *ring;
> +static uint16_t portid;
> +static struct rte_ring *ring;
> 
> -struct rte_metric_name lat_stats_strings[] = {
> +static struct rte_metric_name lat_stats_strings[] = {
>  	{"min_latency_ns"},
>  	{"avg_latency_ns"},
>  	{"max_latency_ns"},
> diff --git a/app/test/test_member_perf.c b/app/test/test_member_perf.c
> index 564a2b3c1..e2840f12d 100644
> --- a/app/test/test_member_perf.c
> +++ b/app/test/test_member_perf.c
> @@ -65,18 +65,18 @@ static uint32_t hashtest_key_lens[] = {  };
> 
>  /* Array to store number of cycles per operation */ -uint64_t
> cycles[NUM_TYPE][NUM_KEYSIZES][NUM_OPERATIONS];
> -uint64_t false_data[NUM_TYPE][NUM_KEYSIZES];
> -uint64_t false_data_bulk[NUM_TYPE][NUM_KEYSIZES];
> -uint64_t false_data_multi[NUM_TYPE][NUM_KEYSIZES];
> -uint64_t false_data_multi_bulk[NUM_TYPE][NUM_KEYSIZES];
> +static uint64_t cycles[NUM_TYPE][NUM_KEYSIZES][NUM_OPERATIONS];
> +static uint64_t false_data[NUM_TYPE][NUM_KEYSIZES];
> +static uint64_t false_data_bulk[NUM_TYPE][NUM_KEYSIZES];
> +static uint64_t false_data_multi[NUM_TYPE][NUM_KEYSIZES];
> +static uint64_t false_data_multi_bulk[NUM_TYPE][NUM_KEYSIZES];
> 
> -uint64_t false_hit[NUM_TYPE][NUM_KEYSIZES];
> +static uint64_t false_hit[NUM_TYPE][NUM_KEYSIZES];
> 
> -member_set_t data[NUM_TYPE][/* Array to store the data */KEYS_TO_ADD];
> +static member_set_t data[NUM_TYPE][/* Array to store the data
> +*/KEYS_TO_ADD];
> 
>  /* Array to store all input keys */
> -uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> +static uint8_t keys[KEYS_TO_ADD][MAX_KEYSIZE];
> 
>  /* Shuffle the keys that have been added, so lookups will be totally random
> */  static void diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
> index d1b9e46a2..53c46ccc1 100644
> --- a/app/test/test_rcu_qsbr.c
> +++ b/app/test/test_rcu_qsbr.c
> @@ -25,8 +25,8 @@
>  /* Make sure that this has the same value as __RTE_QSBR_CNT_INIT */
> #define TEST_RCU_QSBR_CNT_INIT 1
> 
> -uint16_t enabled_core_ids[RTE_MAX_LCORE]; -unsigned int num_cores;
> +static uint16_t enabled_core_ids[RTE_MAX_LCORE]; static unsigned int
> +num_cores;
> 
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -35,8 +35,8 @@ static uint32_t
> *hash_data[RTE_MAX_LCORE][TOTAL_ENTRY];
>  static uint8_t writer_done;
> 
>  static struct rte_rcu_qsbr *t[RTE_MAX_LCORE]; -struct rte_hash
> *h[RTE_MAX_LCORE]; -char hash_name[RTE_MAX_LCORE][8];
> +static struct rte_hash *h[RTE_MAX_LCORE]; static char
> +hash_name[RTE_MAX_LCORE][8];
> 
>  struct test_rcu_thread_info {
>  	/* Index in RCU array */
> @@ -46,7 +46,7 @@ struct test_rcu_thread_info {
>  	/* lcore IDs registered on the RCU variable */
>  	uint16_t r_core_ids[2];
>  };
> -struct test_rcu_thread_info thread_info[RTE_MAX_LCORE/4];
> +static struct test_rcu_thread_info thread_info[RTE_MAX_LCORE/4];
> 
>  static int
>  alloc_rcu(void)
RCU changes look fine.

> --
> 2.21.0

Re: [dpdk-dev] [PATCH 00/10] fix global variable multiple definitions

[David Marchand]

On Thu, Sep 5, 2019 at 4:53 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> Issue has been detected by '-fno-common' gcc flag. By default compiler
> still can figure out that multiple definition are the same variable and
> use same storage for all definitions but this is implementation specific
> behaviour and better to fix it.
>
> Many of the cases below it is nice to have to use 'extern' keyword but
> there are some defects in 'virtio, ''dpaa2_sec' & 'test' that multiple
> components share same global variable unintentionally.
>
> Ferruh Yigit (10):
>   bus/fslmc: fix global variable multiple definitions
>   net/igb: fix global variable multiple definitions
>   crypto/null: fix global variable multiple definitions
>   crypto/octeontx: fix global variable multiple definitions

Dropped this patch, in favor of:
98c7b9c97e32 ("crypto/octeontx: fix global log variable definition")

>   crypto/dpaa2_sec: fix global variable multiple definitions

Hit a build issue (sent a separate mail), dropped this patch.

>   crypto/virtio: fix global variable multiple definitions
>   compress/octeontx: fix global variable multiple definitions
>   app/testpmd: fix global variable multiple definitions
>   app/test-pipeline: fix global variable multiple definitions

And as discussed, dropped this patch on test-pipeline as well.

>   test: fix global variable multiple definitions

My tests on master are ok, so going for it.


Applied, thanks.


--
David Marchand