Re: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]

On Sat, Oct 08, 2016 at 09:35:06AM -0400, Doug Ledford wrote:
> On 10/7/2016 10:24 AM, Leon Romanovsky wrote:
> > On Fri, Oct 07, 2016 at 10:47:18AM +0000, Elior, Ariel wrote:
> >>>> @@ -66,6 +85,12 @@ struct rdma_cqe_common {
> >>>>     struct regpair qp_handle;
> >>>>     __le16 reserved1[7];
> >>>>     u8 flags;
> >>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_MASK  0x1
> >>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_SHIFT 0
> >>>> +#define RDMA_CQE_COMMON_TYPE_MASK        0x3
> >>>> +#define RDMA_CQE_COMMON_TYPE_SHIFT       1
> >>>> +#define RDMA_CQE_COMMON_RESERVED2_MASK   0x1F
> >>>> +#define RDMA_CQE_COMMON_RESERVED2_SHIFT  3
> >>>>     u8 status;
> >>>
> >>> It is VERY uncommon to mix defines and structs together.
> >>> Please don't do it, it confuses a lot and doesn't help to
> >>> readability/debug.
> >>
> >> Hi Leon,
> >> Firstly, thanks for investing your time in reviewing our driver.
> >> As for mixed defines and structures, far from being very uncommon, they are
> >> actually ubiquitous throughout the kernel and are used by the foremost
> >> developers (Dave Miller, Linus, Jeff Kirsher).
> >
> > Net subsystem is very different from other kernel community.
> > For example, this article from LWN [1] - "Coding-style exceptionalism"
> > talks about it.
>
> > [1] https://lwn.net/Articles/694755/
>
> That article only refers to multi-line comments, not to embedding
> #defines inside of structs that the #defines are used with.

That article supports my claim that net subsystem is different from the
rest of the kernel.

>
> My personal taste on things like this is that if you had something like
> a variable with a result code, then use a separate enum for the possible
> options.  However, in this case, you have a multi-mask item and the
> defines are the three masks and their shifts.  I'm OK with that being
> mixed in or being separate, but if it's separate, I would want it
> immediately before the struct with a comment specifying that this is the
> format of the status byte in the struct.  What I wouldn't want is the
> #defines moved far away from the struct with a bunch of other defines
> where the context of the struct is lost.

It looks like you are neutral on the topic, and I'm against mixing these
specific defines with structures. Every change in such define changes
struct as well which can be easily missed out.

Ram,
Please invest an extra effort and help the reviewers to accomplish their
task.

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]