| Message ID | 20170713133935.1206-1-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2017-07-13 at 16:39 +0300, Leon Romanovsky wrote: > diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c > index 1fe79671..da0fe733 100644 > --- a/providers/bnxt_re/verbs.c > +++ b/providers/bnxt_re/verbs.c > @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 13, 2017 at 03:06:11PM +0000, Bart Van Assche wrote: > On Thu, 2017-07-13 at 16:39 +0300, Leon Romanovsky wrote: > > diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c > > index 1fe79671..da0fe733 100644 > > --- a/providers/bnxt_re/verbs.c > > +++ b/providers/bnxt_re/verbs.c > > @@ -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? Yes, better to have consistency. I took this "FALLTHROUGH" from kernel source. I'll resend. Thanks > > Bart.
On Thu, Jul 13, 2017 at 04:39:35PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > [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: Can you follow the same approach as I used in commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5 Author: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Date: Tue Jan 31 14:55:50 2017 -0700 Annotate correct switch fall throughs with SWITCH_FALLTHROUGH Or convert those cases to this approach? > * 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-07-13 at 11:51 -0600, Jason Gunthorpe wrote: > Can you follow the same approach as I used in > > commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5 > Author: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Date: Tue Jan 31 14:55:50 2017 -0700 > > Annotate correct switch fall throughs with SWITCH_FALLTHROUGH > > Or convert those cases to this approach? > > > * 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 13, 2017 at 05:54:12PM +0000, Bart Van Assche wrote: > On Thu, 2017-07-13 at 11:51 -0600, Jason Gunthorpe wrote: > > Can you follow the same approach as I used in > > > > commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5 > > Author: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > Date: Tue Jan 31 14:55:50 2017 -0700 > > > > Annotate correct switch fall throughs with SWITCH_FALLTHROUGH > > > > Or convert those cases to this approach? > > > > > * 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? 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 13, 2017 at 12:15:36PM -0600, Jason Gunthorpe wrote: > On Thu, Jul 13, 2017 at 05:54:12PM +0000, Bart Van Assche wrote: > > On Thu, 2017-07-13 at 11:51 -0600, Jason Gunthorpe wrote: > > > Can you follow the same approach as I used in > > > > > > commit 47cd2efcd4cd416ead0a1a48153aa27a2e178ce5 > > > Author: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > > Date: Tue Jan 31 14:55:50 2017 -0700 > > > > > > Annotate correct switch fall throughs with SWITCH_FALLTHROUGH > > > > > > Or convert those cases to this approach? > > > > > > > * 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? > > 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 > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 13, 2017 at 10:08:29PM +0300, Leon Romanovsky wrote: > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c index 1fe79671..da0fe733 100644 --- a/providers/bnxt_re/verbs.c +++ b/providers/bnxt_re/verbs.c @@ -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;