All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Gal Pressman <galpress@amazon.com>
Cc: Doug Ledford <dledford@redhat.com>,
	linux-rdma@vger.kernel.org,
	Alexander Matushevsky <matua@amazon.com>,
	Daniel Kranzdorf <dkkranzd@amazon.com>,
	Firas JahJah <firasj@amazon.com>
Subject: Re: [PATCH for-next 1/6] RDMA/efa: Unified getters/setters for device structs bitmask access
Date: Wed, 15 Jan 2020 15:31:16 -0400	[thread overview]
Message-ID: <20200115193116.GA11226@ziepe.ca> (raw)
In-Reply-To: <20200114085706.82229-2-galpress@amazon.com>

On Tue, Jan 14, 2020 at 10:57:01AM +0200, Gal Pressman wrote:
> diff --git a/drivers/infiniband/hw/efa/efa_common_defs.h b/drivers/infiniband/hw/efa/efa_common_defs.h
> index c559ec08898e..845ea5ca9388 100644
> +++ b/drivers/infiniband/hw/efa/efa_common_defs.h
> @@ -9,6 +9,12 @@
>  #define EFA_COMMON_SPEC_VERSION_MAJOR        2
>  #define EFA_COMMON_SPEC_VERSION_MINOR        0
>  
> +#define EFA_GET(ptr, type) \
> +	((*(ptr) & type##_MASK) >> type##_SHIFT)
> +
> +#define EFA_SET(ptr, type, value) \
> +	({ *(ptr) |= ((value) << type##_SHIFT) & type##_MASK; })
> +

Why not just GENMASK properly? You don't need MASK and SHIFT, it is
supposed to be written like:

  #define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN GENMASK(8,7)

  *ptr |= FIELD_PREP(val, EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN)

FIELD_PREP automatically deduces the correct shift.

And it would be much nicer if this had some type safety.

You should review the stuff Leon is prepping here:

https://github.com/jgunthorpe/linux/commit/453e85ed7aa46db22d8be16f9b0c88b17b8968af

Which is basically doing the same sorts of things, but with better
type safety and no need for the various structs

Jason

  reply	other threads:[~2020-01-15 19:31 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14  8:57 [PATCH for-next 0/6] EFA updates 2020-01-14 Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 1/6] RDMA/efa: Unified getters/setters for device structs bitmask access Gal Pressman
2020-01-15 19:31   ` Jason Gunthorpe [this message]
2020-01-16  7:05     ` Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 2/6] RDMA/efa: Properly document the interrupt mask register Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 3/6] RDMA/efa: Device definitions documentation updates Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 4/6] RDMA/efa: Remove {} brackets from single statement if Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 5/6] RDMA/efa: Remove unused ucontext parameter from efa_qp_user_mmap_entries_remove Gal Pressman
2020-01-14  8:57 ` [PATCH for-next 6/6] RDMA/efa: Do not delay freeing of DMA pages Gal Pressman
2020-01-15 19:51   ` Jason Gunthorpe
2020-01-16  8:26     ` Gal Pressman
2020-01-15 19:58 ` [PATCH for-next 0/6] EFA updates 2020-01-14 Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200115193116.GA11226@ziepe.ca \
    --to=jgg@ziepe.ca \
    --cc=dkkranzd@amazon.com \
    --cc=dledford@redhat.com \
    --cc=firasj@amazon.com \
    --cc=galpress@amazon.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=matua@amazon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.