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.
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.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG Key ID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
