[05/10] crypto/dpaa2_sec: fix global variable multiple definitions
Checks
Commit Message
'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(-)
Comments
Acked-by: Sachin Saxena <sachin.saxena@nxp.com>
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'.
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?
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).
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.
@@ -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 */
@@ -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>
@@ -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>
@@ -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;
@@ -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
@@ -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 },
@@ -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,
@@ -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 },
@@ -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,
@@ -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,
@@ -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 },
@@ -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 },
@@ -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 },
@@ -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)
{
@@ -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)
{
@@ -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,
@@ -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 },
@@ -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;