Re: [PATCH for-next v12 01/11] RDMA/rxe: Replace #define by enum

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Mar 17, 2022 at 08:55:04PM -0500, Bob Pearson wrote:
> Currently the #define IB_SRQ_INIT_MASK is used to distinguish
> the rxe_create_srq verb from the rxe_modify_srq verb so that some code
> can be shared between these two subroutines.
> 
> This commit replaces the #define with an enum which extends
> enum ib_srq_attr_mask and makes related changes to prototypes
> to clean up type warnings. The parameter is given a rxe specific
> name.
> 
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>  drivers/infiniband/sw/rxe/rxe_loc.h   |  8 ++------
>  drivers/infiniband/sw/rxe/rxe_srq.c   | 10 +++++-----
>  drivers/infiniband/sw/rxe/rxe_verbs.c |  7 ++++---
>  drivers/infiniband/sw/rxe/rxe_verbs.h |  6 ++++++
>  4 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index 2ffbe3390668..9067d3b6f1ee 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -159,17 +159,13 @@ void retransmit_timer(struct timer_list *t);
>  void rnr_nak_timer(struct timer_list *t);
>  
>  /* rxe_srq.c */
> -#define IB_SRQ_INIT_MASK (~IB_SRQ_LIMIT)
> -
>  int rxe_srq_chk_attr(struct rxe_dev *rxe, struct rxe_srq *srq,
> -		     struct ib_srq_attr *attr, enum ib_srq_attr_mask mask);
> -
> +		     struct ib_srq_attr *attr, enum rxe_srq_attr_mask
> mask);

It is very old code that is passing around a enum to indicate a
bitfield, this is not the correct way.

Please just change these to unsigned int.

> +enum rxe_srq_attr_mask {
> +	RXE_SRQ_MAX_WR		= IB_SRQ_MAX_WR,
> +	RXE_SRQ_LIMIT		= IB_SRQ_LIMIT,
> +	RXE_SRQ_INIT		= BIT(2),
> +};

But this seems pretty dodgy?

Why not add another parameter, or maybe split the function or
something?

Jason