* Re: [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for
2020-02-17 22:26 [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for Thomas Monjalon
@ 2020-03-23 13:40 ` David Marchand
2020-03-23 13:47 ` Thomas Monjalon
2020-03-23 13:59 ` Bruce Richardson
2020-05-24 17:30 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-03-23 13:40 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Vladimir Medvedkin, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
On Mon, Feb 17, 2020 at 11:27 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
>
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
>
> The occurrences of the checked pattern are fixed:
> 'for *(\(char\|u\?int\|unsigned\)'
You missed size_t:
drivers/common/mlx5/mlx5_glue.c: for (size_t i = 0; i < num_actions; i++)
> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> app/test-fib/main.c | 3 ++-
> devtools/checkpatches.sh | 8 +++++++
> doc/guides/prog_guide/eventdev.rst | 6 ++++--
> drivers/common/mlx5/mlx5_devx_cmds.c | 4 ++--
> drivers/crypto/caam_jr/caam_jr.c | 5 ++++-
> drivers/net/dpaa2/dpaa2_sparser.c | 30 --------------------------
> lib/librte_eal/windows/eal/eal_lcore.c | 9 ++++----
> 7 files changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/app/test-fib/main.c b/app/test-fib/main.c
> index 3c29ca461e..16d40a6a02 100644
> --- a/app/test-fib/main.c
> +++ b/app/test-fib/main.c
> @@ -363,6 +363,7 @@ static void
> gen_random_rt_6(struct rt_rule_6 *rt, int nh_sz)
> {
> uint32_t i, j, k = 0;
> + int a;
>
> if (config.nb_routes_per_depth[0] != 0) {
> memset(rt[k].addr, 0, 16);
> @@ -370,7 +371,7 @@ gen_random_rt_6(struct rt_rule_6 *rt, int nh_sz)
> rt[k++].nh = rte_rand() & get_max_nh(nh_sz);
> }
>
> - for (int a = 0; a < 4; a++) {
> + for (a = 0; a < 4; a++) {
> for (i = 1; i <= 32; i++) {
> uint32_t rnd;
> double edge = 0;
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index b16bace927..55e7ba05a0 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -61,6 +61,14 @@ check_forbidden_additions() { # <patch>
> -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> "$1" || res=1
>
> + # forbid variable declaration inside "for" loop
> + awk -v FOLDERS='.' \
> + -v EXPRESSIONS='for *\\((char|u?int|unsigned)' \
+ s?size_t
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Declaring a variable inside for()' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
Just a note, base drivers imported "as is" will probably trigger warnings.
> # svg figures must be included with wildcard extension
> # because of png conversion for pdf docs
> awk -v FOLDERS='doc' \
> diff --git a/doc/guides/prog_guide/eventdev.rst b/doc/guides/prog_guide/eventdev.rst
> index 7bcd7603b1..ccde086f63 100644
> --- a/doc/guides/prog_guide/eventdev.rst
> +++ b/doc/guides/prog_guide/eventdev.rst
> @@ -242,9 +242,10 @@ Once queues are set up successfully, create the ports as required.
> };
> int dev_id = 0;
> int rx_port_id = 0;
> + int worker_port_id;
> int err = rte_event_port_setup(dev_id, rx_port_id, &rx_conf);
>
> - for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> + for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> int err = rte_event_port_setup(dev_id, worker_port_id, &worker_conf);
> }
>
> @@ -277,8 +278,9 @@ can be achieved like this:
> uint8_t atomic_qs[] = {0, 1};
> uint8_t single_link_q = 2;
> uint8_t priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
> + int worker_port_id;
>
> - for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> + for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> int links_made = rte_event_port_link(dev_id, worker_port_id, atomic_qs, NULL, 2);
> }
> int links_made = rte_event_port_link(dev_id, tx_port_id, &single_link_q, &priority, 1);
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
> index d960bc93de..ae9dba2e0c 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -412,7 +412,7 @@ mlx5_devx_cmd_query_hca_attr(struct ibv_context *ctx,
> uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
> uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
> void *hcattr;
> - int status, syndrome, rc;
> + int status, syndrome, rc, i;
>
> MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
> MLX5_SET(query_hca_cap_in, in, op_mod,
> @@ -521,7 +521,7 @@ mlx5_devx_cmd_query_hca_attr(struct ibv_context *ctx,
> attr->lro_max_msg_sz_mode = MLX5_GET
> (per_protocol_networking_offload_caps,
> hcattr, lro_max_msg_sz_mode);
> - for (int i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
> + for (i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
> attr->lro_timer_supported_periods[i] =
> MLX5_GET(per_protocol_networking_offload_caps, hcattr,
> lro_timer_supported_periods[i]);
> diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
> index 8aaa3d45f6..9aa64ab23f 100644
> --- a/drivers/crypto/caam_jr/caam_jr.c
> +++ b/drivers/crypto/caam_jr/caam_jr.c
> @@ -1353,6 +1353,9 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
> struct caam_jr_session *ses;
> struct caam_jr_op_ctx *ctx = NULL;
> struct sec_job_descriptor_t *jobdescr __rte_unused;
> +#if CAAM_JR_DBG
> + int i;
> +#endif
>
> switch (op->sess_type) {
> case RTE_CRYPTO_OP_WITH_SESSION:
> @@ -1415,7 +1418,7 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
> rte_pktmbuf_data_len(op->sym->m_src));
>
> printf("\n JD before conversion\n");
> - for (int i = 0; i < 12; i++)
> + for (i = 0; i < 12; i++)
> printf("\n 0x%08x", ctx->jobdes.desc[i]);
> #endif
>
> diff --git a/drivers/net/dpaa2/dpaa2_sparser.c b/drivers/net/dpaa2/dpaa2_sparser.c
> index 7e8fedd818..ba0d500f74 100644
> --- a/drivers/net/dpaa2/dpaa2_sparser.c
> +++ b/drivers/net/dpaa2/dpaa2_sparser.c
> @@ -145,36 +145,6 @@ struct frame_attr_ext frame_attr_ext_arr[] = {
> /* 112 */ {NULL, 0, 0x0000}
> };
>
> -#define SWAP_WORD(pr) \
> -do { \
> - for (int i = 0; i < 4 ; i++) { \
> - pr[i] = pr[i] ^ pr[6 - i + 1]; \
> - pr[6 - i + 1] = pr[6 - i + 1] ^ pr[i]; \
> - pr[i] = pr[i] ^ pr[6 - i + 1]; \
> - } \
> -} while (0)
> -
> -#define fa_print_sb() \
> -do { \
> - if (rte_cpu_to_be_32(*pdw) & frm_attr->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("t %s : Yes", frm_attr->fld_name); \
> -} while (0)
> -
> -#define fa_print_sb_ext() \
> -do { \
> - if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("\t %s : Yes", \
> - frm_attr_ext->fld_name); \
> -} while (0)
> -
> -#define fa_print_mb_ext() \
> -do { \
> - if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("\t %s : 0x%02x", \
> - frm_attr_ext->fld_name, \
> - rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask);\
> -} while (0)
> -
> int dpaa2_eth_load_wriop_soft_parser(struct dpaa2_dev_priv *priv,
> enum dpni_soft_sequence_dest dest)
> {
> diff --git a/lib/librte_eal/windows/eal/eal_lcore.c b/lib/librte_eal/windows/eal/eal_lcore.c
> index b3a6c63afa..8ee1d6fb78 100644
> --- a/lib/librte_eal/windows/eal/eal_lcore.c
> +++ b/lib/librte_eal/windows/eal/eal_lcore.c
> @@ -27,6 +27,8 @@ static struct _wcpu_map {
> void
> eal_create_cpu_map()
> {
> + unsigned int socket, core, lcore;
> +
> wcpu_map.total_procs =
> GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
>
> @@ -53,10 +55,9 @@ eal_create_cpu_map()
> * across the logical cores. For now, split the cores
> * equally across the sockets.
> */
> - unsigned int lcore = 0;
> - for (unsigned int socket = 0; socket <
> - wcpu_map.proc_sockets; ++socket) {
> - for (unsigned int core = 0;
> + lcore = 0;
> + for (socket = 0; socket < wcpu_map.proc_sockets; ++socket) {
> + for (core = 0;
> core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
> ++core) {
> wcpu_map.wlcore_map[lcore]
> --
> 2.25.0
>
LGTM once size_t is fixed.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for
2020-03-23 13:40 ` David Marchand
@ 2020-03-23 13:47 ` Thomas Monjalon
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2020-03-23 13:47 UTC (permalink / raw)
To: David Marchand
Cc: dev, Vladimir Medvedkin, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
23/03/2020 14:40, David Marchand:
> On Mon, Feb 17, 2020 at 11:27 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > Some compilers raise an error when declaring a variable
> > in the middle of a function. This is a C99 allowance.
> > Even if DPDK switches globally to C99 or C11 standard,
> > the coding rules are for declarations at the beginning
> > of a block:
> > http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
> >
> > This coding style is enforced by adding a check of
> > the common patterns like "for (int i;"
> >
> > The occurrences of the checked pattern are fixed:
> > 'for *(\(char\|u\?int\|unsigned\)'
>
> You missed size_t:
> drivers/common/mlx5/mlx5_glue.c: for (size_t i = 0; i < num_actions; i++)
Good catch, thanks.
[..]
> > --- a/devtools/checkpatches.sh
> > +++ b/devtools/checkpatches.sh
> > + # forbid variable declaration inside "for" loop
> > + awk -v FOLDERS='.' \
> > + -v EXPRESSIONS='for *\\((char|u?int|unsigned)' \
>
> + s?size_t
OK
> > + -v RET_ON_FAIL=1 \
> > + -v MESSAGE='Declaring a variable inside for()' \
> > + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > + "$1" || res=1
> > +
>
> Just a note, base drivers imported "as is" will probably trigger warnings.
Yes, base drivers don't comply with coding rules and trigger lots of warnings.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for
2020-02-17 22:26 [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for Thomas Monjalon
2020-03-23 13:40 ` David Marchand
@ 2020-03-23 13:59 ` Bruce Richardson
2020-03-23 16:12 ` Thomas Monjalon
2020-05-24 17:30 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2 siblings, 1 reply; 9+ messages in thread
From: Bruce Richardson @ 2020-03-23 13:59 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Vladimir Medvedkin, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
On Mon, Feb 17, 2020 at 11:26:54PM +0100, Thomas Monjalon wrote:
> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
>
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
>
Could we, or should we, not also revisit the coding standards for this.
Those standards are quite old now, and could do with being updated to take
account of things like the newer C standards. Obviously for consistency
reasons any changes should not be major, but I don't think we should view
the guidelines as set in stone either.
For this particular issue, I think generally having all declarations at the
beginning of a block is the right thing to do, however, I would support
relaxing this rule in a couple of scenarios, specifically:
* declaring a variable like i in a for loop, i.e. the case above
* using const on a variable declaration, marking it as const at first
assignment [because I take the view that increased use of const is almost
always a good thing]
Regards,
/Bruce
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for
2020-03-23 13:59 ` Bruce Richardson
@ 2020-03-23 16:12 ` Thomas Monjalon
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2020-03-23 16:12 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, Vladimir Medvedkin, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
23/03/2020 14:59, Bruce Richardson:
> On Mon, Feb 17, 2020 at 11:26:54PM +0100, Thomas Monjalon wrote:
> > Some compilers raise an error when declaring a variable
> > in the middle of a function. This is a C99 allowance.
> > Even if DPDK switches globally to C99 or C11 standard,
> > the coding rules are for declarations at the beginning
> > of a block:
> > http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
> >
> > This coding style is enforced by adding a check of
> > the common patterns like "for (int i;"
> >
>
> Could we, or should we, not also revisit the coding standards for this.
> Those standards are quite old now, and could do with being updated to take
> account of things like the newer C standards. Obviously for consistency
> reasons any changes should not be major, but I don't think we should view
> the guidelines as set in stone either.
>
> For this particular issue, I think generally having all declarations at the
> beginning of a block is the right thing to do, however, I would support
> relaxing this rule in a couple of scenarios, specifically:
>
> * declaring a variable like i in a for loop, i.e. the case above
> * using const on a variable declaration, marking it as const at first
> assignment [because I take the view that increased use of const is almost
> always a good thing]
I would tend to agree in general for updating guidelines,
and especially in this case.
But because we are not enforcing C99 (or C11) when compiling,
some users are still trying to compile with old compilers.
As a result, a declaration in a for loop will break compilation
with some compilers:
http://git.dpdk.org/dpdk/commit/?id=bc3282125b
I am OK either way:
- switch globally to C99/C11
- or forbid declaration in for loop
I took the second option because it is easier,
and because it maximizes our compiler tolerance.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [dpdk-dev] [PATCH v2] devtools: forbid variable declaration inside for
2020-02-17 22:26 [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for Thomas Monjalon
2020-03-23 13:40 ` David Marchand
2020-03-23 13:59 ` Bruce Richardson
@ 2020-05-24 17:30 ` Thomas Monjalon
2020-05-24 18:30 ` Stephen Hemminger
2020-05-28 12:30 ` David Marchand
2 siblings, 2 replies; 9+ messages in thread
From: Thomas Monjalon @ 2020-05-24 17:30 UTC (permalink / raw)
To: dev
Cc: david.marchand, bruce.richardson, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
Some compilers raise an error when declaring a variable
in the middle of a function. This is a C99 allowance.
Even if DPDK switches globally to C99 or C11 standard,
the coding rules are for declarations at the beginning
of a block:
http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
This coding style is enforced by adding a check of
the common patterns like "for (int i;"
The occurrences of the checked pattern are fixed:
'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
In the file dpaa2_sparser.c, the fix is to remove the unused macros.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
v2:
- check s?size_t and fix one occurence
- fix new occurences in Windows EAL
- test-fib has been fixed in the meantime
---
devtools/checkpatches.sh | 8 ++++++++
doc/guides/prog_guide/eventdev.rst | 6 ++++--
drivers/common/mlx5/mlx5_devx_cmds.c | 4 ++--
drivers/common/mlx5/mlx5_glue.c | 3 ++-
drivers/crypto/caam_jr/caam_jr.c | 5 ++++-
drivers/net/dpaa2/dpaa2_sparser.c | 30 ----------------------------
lib/librte_eal/windows/eal_lcore.c | 6 ++++--
7 files changed, 24 insertions(+), 38 deletions(-)
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 42b833e0d7..5763a7e953 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -69,6 +69,14 @@ check_forbidden_additions() { # <patch>
-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
"$1" || res=1
+ # forbid variable declaration inside "for" loop
+ awk -v FOLDERS='.' \
+ -v EXPRESSIONS='for *\\((char|u?int|unsigned|s?size_t)' \
+ -v RET_ON_FAIL=1 \
+ -v MESSAGE='Declaring a variable inside for()' \
+ -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+ "$1" || res=1
+
# svg figures must be included with wildcard extension
# because of png conversion for pdf docs
awk -v FOLDERS='doc' \
diff --git a/doc/guides/prog_guide/eventdev.rst b/doc/guides/prog_guide/eventdev.rst
index 7bcd7603b1..ccde086f63 100644
--- a/doc/guides/prog_guide/eventdev.rst
+++ b/doc/guides/prog_guide/eventdev.rst
@@ -242,9 +242,10 @@ Once queues are set up successfully, create the ports as required.
};
int dev_id = 0;
int rx_port_id = 0;
+ int worker_port_id;
int err = rte_event_port_setup(dev_id, rx_port_id, &rx_conf);
- for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
+ for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
int err = rte_event_port_setup(dev_id, worker_port_id, &worker_conf);
}
@@ -277,8 +278,9 @@ can be achieved like this:
uint8_t atomic_qs[] = {0, 1};
uint8_t single_link_q = 2;
uint8_t priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
+ int worker_port_id;
- for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
+ for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
int links_made = rte_event_port_link(dev_id, worker_port_id, atomic_qs, NULL, 2);
}
int links_made = rte_event_port_link(dev_id, tx_port_id, &single_link_q, &priority, 1);
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index fba485e724..1c8b36c91a 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -416,7 +416,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
void *hcattr;
- int status, syndrome, rc;
+ int status, syndrome, rc, i;
MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
MLX5_SET(query_hca_cap_in, in, op_mod,
@@ -529,7 +529,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
attr->lro_max_msg_sz_mode = MLX5_GET
(per_protocol_networking_offload_caps,
hcattr, lro_max_msg_sz_mode);
- for (int i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
+ for (i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
attr->lro_timer_supported_periods[i] =
MLX5_GET(per_protocol_networking_offload_caps, hcattr,
lro_timer_supported_periods[i]);
diff --git a/drivers/common/mlx5/mlx5_glue.c b/drivers/common/mlx5/mlx5_glue.c
index f270f677b7..563941c2df 100644
--- a/drivers/common/mlx5/mlx5_glue.c
+++ b/drivers/common/mlx5/mlx5_glue.c
@@ -587,11 +587,12 @@ mlx5_glue_dv_create_flow(void *matcher,
return mlx5dv_dr_rule_create(matcher, match_value, num_actions,
(struct mlx5dv_dr_action **)actions);
#else
+ size_t i;
struct mlx5dv_flow_action_attr actions_attr[8];
if (num_actions > 8)
return NULL;
- for (size_t i = 0; i < num_actions; i++)
+ for (i = 0; i < num_actions; i++)
actions_attr[i] =
*((struct mlx5dv_flow_action_attr *)(actions[i]));
return mlx5dv_create_flow(matcher, match_value,
diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
index caf2386772..77fa6ff2d3 100644
--- a/drivers/crypto/caam_jr/caam_jr.c
+++ b/drivers/crypto/caam_jr/caam_jr.c
@@ -1351,6 +1351,9 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
struct caam_jr_session *ses;
struct caam_jr_op_ctx *ctx = NULL;
struct sec_job_descriptor_t *jobdescr __rte_unused;
+#if CAAM_JR_DBG
+ int i;
+#endif
switch (op->sess_type) {
case RTE_CRYPTO_OP_WITH_SESSION:
@@ -1413,7 +1416,7 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
rte_pktmbuf_data_len(op->sym->m_src));
printf("\n JD before conversion\n");
- for (int i = 0; i < 12; i++)
+ for (i = 0; i < 12; i++)
printf("\n 0x%08x", ctx->jobdes.desc[i]);
#endif
diff --git a/drivers/net/dpaa2/dpaa2_sparser.c b/drivers/net/dpaa2/dpaa2_sparser.c
index 7e8fedd818..ba0d500f74 100644
--- a/drivers/net/dpaa2/dpaa2_sparser.c
+++ b/drivers/net/dpaa2/dpaa2_sparser.c
@@ -145,36 +145,6 @@ struct frame_attr_ext frame_attr_ext_arr[] = {
/* 112 */ {NULL, 0, 0x0000}
};
-#define SWAP_WORD(pr) \
-do { \
- for (int i = 0; i < 4 ; i++) { \
- pr[i] = pr[i] ^ pr[6 - i + 1]; \
- pr[6 - i + 1] = pr[6 - i + 1] ^ pr[i]; \
- pr[i] = pr[i] ^ pr[6 - i + 1]; \
- } \
-} while (0)
-
-#define fa_print_sb() \
-do { \
- if (rte_cpu_to_be_32(*pdw) & frm_attr->fld_mask) \
- DPAA2_PMD_DP_DEBUG("t %s : Yes", frm_attr->fld_name); \
-} while (0)
-
-#define fa_print_sb_ext() \
-do { \
- if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
- DPAA2_PMD_DP_DEBUG("\t %s : Yes", \
- frm_attr_ext->fld_name); \
-} while (0)
-
-#define fa_print_mb_ext() \
-do { \
- if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
- DPAA2_PMD_DP_DEBUG("\t %s : 0x%02x", \
- frm_attr_ext->fld_name, \
- rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask);\
-} while (0)
-
int dpaa2_eth_load_wriop_soft_parser(struct dpaa2_dev_priv *priv,
enum dpni_soft_sequence_dest dest)
{
diff --git a/lib/librte_eal/windows/eal_lcore.c b/lib/librte_eal/windows/eal_lcore.c
index 82ee454134..760b3fcfed 100644
--- a/lib/librte_eal/windows/eal_lcore.c
+++ b/lib/librte_eal/windows/eal_lcore.c
@@ -29,6 +29,8 @@ static struct _wcpu_map {
void
eal_create_cpu_map()
{
+ unsigned int socket, core;
+
wcpu_map.total_procs =
GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
@@ -56,9 +58,9 @@ eal_create_cpu_map()
* equally across the sockets.
*/
unsigned int lcore = 0;
- for (unsigned int socket = 0; socket <
+ for (socket = 0; socket <
wcpu_map.proc_sockets; ++socket) {
- for (unsigned int core = 0;
+ for (core = 0;
core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
++core) {
wcpu_map.wlcore_map[lcore]
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [dpdk-dev] [PATCH v2] devtools: forbid variable declaration inside for
2020-05-24 17:30 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
@ 2020-05-24 18:30 ` Stephen Hemminger
2020-05-28 12:30 ` David Marchand
1 sibling, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2020-05-24 18:30 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, bruce.richardson, John McNamara,
Marko Kovacevic, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Gagandeep Singh, Hemant Agrawal,
Sachin Saxena, Harini Ramakrishnan, Omar Cardona, Pallavi Kadam,
Ranjit Menon
On Sun, 24 May 2020 19:30:07 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:
> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
>
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
>
> The occurrences of the checked pattern are fixed:
> 'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Surprised that checkpatch doesn't enforce this already.
Or maybe kernel flags are different.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] devtools: forbid variable declaration inside for
2020-05-24 17:30 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2020-05-24 18:30 ` Stephen Hemminger
@ 2020-05-28 12:30 ` David Marchand
2020-07-03 9:08 ` David Marchand
1 sibling, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-05-28 12:30 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Bruce Richardson, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
On Sun, May 24, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
>
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
>
> The occurrences of the checked pattern are fixed:
> 'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> v2:
> - check s?size_t and fix one occurence
> - fix new occurences in Windows EAL
> - test-fib has been fixed in the meantime
> ---
> devtools/checkpatches.sh | 8 ++++++++
> doc/guides/prog_guide/eventdev.rst | 6 ++++--
> drivers/common/mlx5/mlx5_devx_cmds.c | 4 ++--
> drivers/common/mlx5/mlx5_glue.c | 3 ++-
> drivers/crypto/caam_jr/caam_jr.c | 5 ++++-
> drivers/net/dpaa2/dpaa2_sparser.c | 30 ----------------------------
> lib/librte_eal/windows/eal_lcore.c | 6 ++++--
> 7 files changed, 24 insertions(+), 38 deletions(-)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 42b833e0d7..5763a7e953 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -69,6 +69,14 @@ check_forbidden_additions() { # <patch>
> -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> "$1" || res=1
>
> + # forbid variable declaration inside "for" loop
> + awk -v FOLDERS='.' \
> + -v EXPRESSIONS='for *\\((char|u?int|unsigned|s?size_t)' \
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Declaring a variable inside for()' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
> # svg figures must be included with wildcard extension
> # because of png conversion for pdf docs
> awk -v FOLDERS='doc' \
> diff --git a/doc/guides/prog_guide/eventdev.rst b/doc/guides/prog_guide/eventdev.rst
> index 7bcd7603b1..ccde086f63 100644
> --- a/doc/guides/prog_guide/eventdev.rst
> +++ b/doc/guides/prog_guide/eventdev.rst
> @@ -242,9 +242,10 @@ Once queues are set up successfully, create the ports as required.
> };
> int dev_id = 0;
> int rx_port_id = 0;
> + int worker_port_id;
> int err = rte_event_port_setup(dev_id, rx_port_id, &rx_conf);
>
> - for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> + for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> int err = rte_event_port_setup(dev_id, worker_port_id, &worker_conf);
> }
>
> @@ -277,8 +278,9 @@ can be achieved like this:
> uint8_t atomic_qs[] = {0, 1};
> uint8_t single_link_q = 2;
> uint8_t priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
> + int worker_port_id;
>
> - for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> + for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> int links_made = rte_event_port_link(dev_id, worker_port_id, atomic_qs, NULL, 2);
> }
> int links_made = rte_event_port_link(dev_id, tx_port_id, &single_link_q, &priority, 1);
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
> index fba485e724..1c8b36c91a 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -416,7 +416,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
> uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
> uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
> void *hcattr;
> - int status, syndrome, rc;
> + int status, syndrome, rc, i;
>
> MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
> MLX5_SET(query_hca_cap_in, in, op_mod,
> @@ -529,7 +529,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
> attr->lro_max_msg_sz_mode = MLX5_GET
> (per_protocol_networking_offload_caps,
> hcattr, lro_max_msg_sz_mode);
> - for (int i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
> + for (i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
> attr->lro_timer_supported_periods[i] =
> MLX5_GET(per_protocol_networking_offload_caps, hcattr,
> lro_timer_supported_periods[i]);
> diff --git a/drivers/common/mlx5/mlx5_glue.c b/drivers/common/mlx5/mlx5_glue.c
> index f270f677b7..563941c2df 100644
> --- a/drivers/common/mlx5/mlx5_glue.c
> +++ b/drivers/common/mlx5/mlx5_glue.c
> @@ -587,11 +587,12 @@ mlx5_glue_dv_create_flow(void *matcher,
> return mlx5dv_dr_rule_create(matcher, match_value, num_actions,
> (struct mlx5dv_dr_action **)actions);
> #else
> + size_t i;
> struct mlx5dv_flow_action_attr actions_attr[8];
>
> if (num_actions > 8)
> return NULL;
> - for (size_t i = 0; i < num_actions; i++)
> + for (i = 0; i < num_actions; i++)
> actions_attr[i] =
> *((struct mlx5dv_flow_action_attr *)(actions[i]));
> return mlx5dv_create_flow(matcher, match_value,
> diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
> index caf2386772..77fa6ff2d3 100644
> --- a/drivers/crypto/caam_jr/caam_jr.c
> +++ b/drivers/crypto/caam_jr/caam_jr.c
> @@ -1351,6 +1351,9 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
> struct caam_jr_session *ses;
> struct caam_jr_op_ctx *ctx = NULL;
> struct sec_job_descriptor_t *jobdescr __rte_unused;
> +#if CAAM_JR_DBG
> + int i;
> +#endif
>
> switch (op->sess_type) {
> case RTE_CRYPTO_OP_WITH_SESSION:
> @@ -1413,7 +1416,7 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
> rte_pktmbuf_data_len(op->sym->m_src));
>
> printf("\n JD before conversion\n");
> - for (int i = 0; i < 12; i++)
> + for (i = 0; i < 12; i++)
> printf("\n 0x%08x", ctx->jobdes.desc[i]);
> #endif
>
> diff --git a/drivers/net/dpaa2/dpaa2_sparser.c b/drivers/net/dpaa2/dpaa2_sparser.c
> index 7e8fedd818..ba0d500f74 100644
> --- a/drivers/net/dpaa2/dpaa2_sparser.c
> +++ b/drivers/net/dpaa2/dpaa2_sparser.c
> @@ -145,36 +145,6 @@ struct frame_attr_ext frame_attr_ext_arr[] = {
> /* 112 */ {NULL, 0, 0x0000}
> };
>
> -#define SWAP_WORD(pr) \
> -do { \
> - for (int i = 0; i < 4 ; i++) { \
> - pr[i] = pr[i] ^ pr[6 - i + 1]; \
> - pr[6 - i + 1] = pr[6 - i + 1] ^ pr[i]; \
> - pr[i] = pr[i] ^ pr[6 - i + 1]; \
> - } \
> -} while (0)
> -
> -#define fa_print_sb() \
> -do { \
> - if (rte_cpu_to_be_32(*pdw) & frm_attr->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("t %s : Yes", frm_attr->fld_name); \
> -} while (0)
> -
> -#define fa_print_sb_ext() \
> -do { \
> - if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("\t %s : Yes", \
> - frm_attr_ext->fld_name); \
> -} while (0)
> -
> -#define fa_print_mb_ext() \
> -do { \
> - if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask) \
> - DPAA2_PMD_DP_DEBUG("\t %s : 0x%02x", \
> - frm_attr_ext->fld_name, \
> - rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask);\
> -} while (0)
> -
> int dpaa2_eth_load_wriop_soft_parser(struct dpaa2_dev_priv *priv,
> enum dpni_soft_sequence_dest dest)
> {
> diff --git a/lib/librte_eal/windows/eal_lcore.c b/lib/librte_eal/windows/eal_lcore.c
> index 82ee454134..760b3fcfed 100644
> --- a/lib/librte_eal/windows/eal_lcore.c
> +++ b/lib/librte_eal/windows/eal_lcore.c
> @@ -29,6 +29,8 @@ static struct _wcpu_map {
> void
> eal_create_cpu_map()
> {
> + unsigned int socket, core;
> +
> wcpu_map.total_procs =
> GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
>
> @@ -56,9 +58,9 @@ eal_create_cpu_map()
> * equally across the sockets.
> */
> unsigned int lcore = 0;
> - for (unsigned int socket = 0; socket <
> + for (socket = 0; socket <
> wcpu_map.proc_sockets; ++socket) {
> - for (unsigned int core = 0;
> + for (core = 0;
> core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
> ++core) {
> wcpu_map.wlcore_map[lcore]
> --
> 2.26.2
>
Acked-by: David Marchand <david.marchand@redhat.com>
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [dpdk-dev] [PATCH v2] devtools: forbid variable declaration inside for
2020-05-28 12:30 ` David Marchand
@ 2020-07-03 9:08 ` David Marchand
0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2020-07-03 9:08 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Bruce Richardson, John McNamara, Marko Kovacevic,
Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
Gagandeep Singh, Hemant Agrawal, Sachin Saxena,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
On Thu, May 28, 2020 at 2:30 PM David Marchand
<david.marchand@redhat.com> wrote:
> On Sun, May 24, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > Some compilers raise an error when declaring a variable
> > in the middle of a function. This is a C99 allowance.
> > Even if DPDK switches globally to C99 or C11 standard,
> > the coding rules are for declarations at the beginning
> > of a block:
> > http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
> >
> > This coding style is enforced by adding a check of
> > the common patterns like "for (int i;"
> >
> > The occurrences of the checked pattern are fixed:
> > 'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> > In the file dpaa2_sparser.c, the fix is to remove the unused macros.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: David Marchand <david.marchand@redhat.com>
Discarded the (now obsolete) change on windows/eal.c and applied.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread