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: 2577 bytes --]

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.

>
> In infiniband tree alone, at least three drivers employ this:
> drivers/infiniband/hw/ocrdma/ocrdma_sli.h line 1900
> drivers/infiniband/hw/mthca/mthca_user.h line 68
> drivers/infiniband/hw/cxgb3/cxio_hal.h line 116

All of them are copy-paste from pre-historic era.

>
> In the net subsystem, it is even more widely used (~14k places), including
> mellanox and intel drivers, as well as our bnx2x and qed* drivers and many
> others. A few examples can be seen under:
> drivers/net/ethernet/mellanox/mlx4/en_port.h line 94
> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h line 345
> drivers/net/ethernet/mellanox/mlx4/fw.c line 2759

Thanks for pointing it.
We will fix it.

> drivers/net/ethernet/intel/ixgbe/ixgbe.h line 623 (Jeff Kirsher)
> drivers/net/ethernet/broadcom/tg3.h line 2540 (Dave Miller)
> ixgbe.h uses this exactly like we do in the code you cited.
>
> In other kernel cornerstones:
> fs/ext4/ext4.h line 1287 (Linus)

1. From git blame, this define was added in 2010 !!!!
2. It has totally different meaning from your code - to mark the
position in the structure.

> include/net/sock.h line 312 (Dave)

Net is a bad example.

>
> I don't think there are grounds for objecting to this style. We'll take care
> of the rest of your comments.

As you wish, at the end it will be Doug's decision, if he wants to see
driver submitted in 2016 in different coding style from rest of the subsystem.

[1] https://lwn.net/Articles/694755/

>
> Thanks,
> Ariel

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