bnxt_re: Fix -Wimplicit-fallthrough warnings in GCC 7.1

PATCH In review Leon Romanovsky 3 participants 8 messages Jul 13, 2017 6 messages not in the archive InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2017-07-13 13:39 UTC #0 lore raw

PATCH 1/1 bnxt_re: Fix -Wimplicit-fallthrough warnings in GCC 7.1

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

[59/189] Building C object providers/bnxt_re/CMakeFiles/bnxt_re-rdmav2.dir/verbs.c.o ../providers/bnxt_re/verbs.c: In function ‘bnxt_re_post_send’:
../providers/bnxt_re/verbs.c:1222:18: warning: this statement may fall through [-Wimplicit-fallthrough=]

    hdr->key_immd = htole32(be32toh(wr->imm_data));
                  ^

../providers/bnxt_re/verbs.c:1223:3: note: here
case IBV_WR_SEND:
^~~~
../providers/bnxt_re/verbs.c:1232:18: warning: this statement may fall through [-Wimplicit-fallthrough=]

    hdr->key_immd = htole32(be32toh(wr->imm_data));
                  ^

../providers/bnxt_re/verbs.c:1233:3: note: here
case IBV_WR_RDMA_WRITE:

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 providers/bnxt_re/verbs.c | 2 ++
 1 file changed, 2 insertions(+)

providers/bnxt_re/verbs.c +2 -0

index 1fe79671..da0fe733 100644
@@ -1220,6 +1220,7 @@ int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr,
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
 		case IBV_WR_SEND:
 			if (qp->qptyp == IBV_QPT_UD)
 				bytes = bnxt_re_build_ud_sqe(qp, sqe, wr,

@@ -1230,6 +1231,7 @@ int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr,
 			break;
 		case IBV_WR_RDMA_WRITE_WITH_IMM:
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
 		case IBV_WR_RDMA_WRITE:
 			bytes = bnxt_re_build_rdma_sqe(qp, sqe, wr, is_inline);
 			break;

Signature · 6 lines

2.13.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170713133935.1206-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Bart Van Assche

2017-07-13 15:06 UTC #2 lore raw

providers/bnxt_re/verbs.c int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr, · line 1223

Quoted 9 lines · Leon Romanovsky

providers/bnxt_re/verbs.c +1 -0

index 1fe79671..da0fe733 100644
@@ -1220,6 +1220,7 @@ int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr,
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */

Hello Leon,

In other rdma-core source files the annotation "/* fall through */" is used. See also the output of git grep -nHi '/\* *fall[ -]*through'. Should we ensure that these annotations are consistent in the rdma-core source tree?

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Leon Romanovsky

author

2017-07-13 15:34 UTC #4 lore raw

Quoted 13 lines · Bart Van Assche

providers/bnxt_re/verbs.c int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr, · line 1223
Quoted 9 lines · Leon Romanovsky
providers/bnxt_re/verbs.c +1 -0
index 1fe79671..da0fe733 100644
@@ -1220,6 +1220,7 @@ int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr,
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
Hello Leon,

Quoted 3 lines

In other rdma-core source files the annotation "/* fall through */" is used. See also the output of git grep -nHi '/\* *fall[ -]*through'. Should we ensure that these annotations are consistent in the rdma-core source tree?

Yes, better to have consistency. I took this "FALLTHROUGH" from kernel source.

I'll resend.

Thanks

Quoted 2 lines

Bart.

Jason Gunthorpe

2017-07-13 17:51 UTC #5 lore raw

In reply to a message not in the archive

Quoted 13 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
[59/189] Building C object providers/bnxt_re/CMakeFiles/bnxt_re-rdmav2.dir/verbs.c.o ../providers/bnxt_re/verbs.c: In function ‘bnxt_re_post_send’:
../providers/bnxt_re/verbs.c:1222:18: warning: this statement may fall through [-Wimplicit-fallthrough=]
    hdr->key_immd = htole32(be32toh(wr->imm_data));
                  ^
../providers/bnxt_re/verbs.c:1223:3: note: here
   case IBV_WR_SEND:
   ^~~~
../providers/bnxt_re/verbs.c:1232:18: warning: this statement may fall through [-Wimplicit-fallthrough=]
    hdr->key_immd = htole32(be32toh(wr->imm_data));
                  ^

Quoted 2 lines

../providers/bnxt_re/verbs.c:1233:3: note: here
case IBV_WR_RDMA_WRITE:

Can you follow the same approach as I used in

commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5

Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date:   Tue Jan 31 14:55:50 2017 -0700

Annotate correct switch fall throughs with SWITCH_FALLTHROUGH

Or convert those cases to this approach?

providers/bnxt_re/verbs.c int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr, · line 1223

Quoted 4 lines

providers/bnxt_re/verbs.c +1 -0

 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */

Use the SWITCH_FALLTHROUGH; macro

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170713175122.GD11069-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2017-07-13 17:54 UTC #7 lore raw

Quoted 8 lines · Jason Gunthorpe

Can you follow the same approach as I used in
commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5
Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date:   Tue Jan 31 14:55:50 2017 -0700
Annotate correct switch fall throughs with SWITCH_FALLTHROUGH

Quoted 8 lines

Or convert those cases to this approach?
providers/bnxt_re/verbs.c int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr, · line 1223
Quoted 4 lines
providers/bnxt_re/verbs.c +1 -0
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
Use the SWITCH_FALLTHROUGH; macro

Hello Jason,

Both Coverity and gcc 7 understand fall-through comments. Do we really need a macro to annotate fall-through in switch statements?

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

2017-07-13 18:15 UTC #9 lore raw

Quoted 18 lines · Bart Van Assche

Quoted 8 lines · Jason Gunthorpe
Can you follow the same approach as I used in
commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5
Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date:   Tue Jan 31 14:55:50 2017 -0700
Annotate correct switch fall throughs with SWITCH_FALLTHROUGH
Quoted 8 lines
Or convert those cases to this approach?
providers/bnxt_re/verbs.c int bnxt_re_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr, · line 1223
Quoted 4 lines
providers/bnxt_re/verbs.c +1 -0
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
Use the SWITCH_FALLTHROUGH; macro

Quoted 4 lines

Hello Jason,
Both Coverity and gcc 7 understand fall-through comments. Do we really need a macro to annotate fall-through in switch statements?

Probably not, but I added it a while ago already based on some other documentation..

The advantage of the macro is that all compilers verify it is spelled correctly, while it is easy to make a mistake with comments..

In any case, let us do it consistently throughtout..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170713181536.GA4771-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-07-13 19:08 UTC #11 lore raw

Quoted 24 lines · Jason Gunthorpe

Quoted 18 lines · Bart Van Assche
Quoted 8 lines · Jason Gunthorpe
Can you follow the same approach as I used in
commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5
Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date:   Tue Jan 31 14:55:50 2017 -0700
Annotate correct switch fall throughs with SWITCH_FALLTHROUGH
Quoted 8 lines
Or convert those cases to this approach?
Review
Quoted 4 lines
(quoted diff) +1 -0
 			 * LE platform be32toh will do the job.
 			 */
 			hdr->key_immd = htole32(be32toh(wr->imm_data));
+			/* FALLTHROUGH */
Use the SWITCH_FALLTHROUGH; macro
Quoted 4 lines
Hello Jason,
Both Coverity and gcc 7 understand fall-through comments. Do we really need a macro to annotate fall-through in switch statements?

Quoted 7 lines

Probably not, but I added it a while ago already based on some other documentation..
The advantage of the macro is that all compilers verify it is spelled correctly, while it is easy to make a mistake with comments..
In any case, let us do it consistently throughtout..

No problem, it is less than 1 minute change.

The thing which bothers me, why didn't we see this warning in Travis CI? We need to treat all compilation warnings as errors.

Thanks

Quoted 6 lines

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170713190829.GQ1528-U/[email protected]

Jason Gunthorpe

2017-07-13 19:24 UTC #13 lore raw

Quoted 2 lines · Leon Romanovsky

The thing which bothers me, why didn't we see this warning in Travis CI? We need to treat all compilation warnings as errors.

Travis does not have gcc 7 right now, the 6.x compiler does not produce the warning by default.

I haven't looked lately if we can switch travis to 7 or not.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html