Re: [PATCH] bitfield: add constant field preparation macros

[John Garry <john.garry@huawei.com>]

On 12/10/2018 20:45, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> John Garry requested to be able to use FIELD_PREP() and friends
> in constant initializers, but we cannot completely switch all of
> the current assertions to BUILD_BUG_ON_ZERO().
>

Thanks for this.

> So instead of this, add __FIELD_PREP() which is suitable in such
> contexts, and also add __{u,le,be}{16,32,64}encode_bits() like
> the existing versions without underscores, but again suitable in
> constant contexts.
>
> Requested-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/linux/bitfield.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 3f1ef4450a7c..245dfb47d201 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -63,6 +63,14 @@
>  					      (1ULL << __bf_shf(_mask))); \
>  	})
>
> +#define __BF_CHECK_POW2(n)	BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
> +
> +#define __BF_FIELD_CHECK_CONST(_mask, _reg, _val)			\
> +	(BUILD_BUG_ON_ZERO((_mask) == 0) +				\
> +	 BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) +	\
> +	 BUILD_BUG_ON_ZERO((_mask) > (typeof(_reg))~0ull) +		\
> +	 __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))))
> +
>  /**
>   * FIELD_FIT() - check if value fits in the field
>   * @_mask: shifted mask defining the field's length and position
> @@ -90,6 +98,21 @@
>  		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
>  	})
>
> +/**
> + * __FIELD_PREP() - prepare a constant bitfield element

My impression is that the name prefix - '__' - tells little about the 
function. If you agree, how about even CFIELD_PREP() or 
FIELD_PREP_CONST() or similar? I preper the latter, but becomes rather long.

> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * __FIELD_PREP() masks and shifts up the value.  The result should
> + * be combined with other fields of the bitfield using local OR.

should this be 'logical OR', or indeed 'bitwise OR'?

> + *
> + * This version is suitable for use in a pure constant context, e.g.
> + * a constant initializer.
> + */
> +#define __FIELD_PREP(_mask, _val)					\
> +	((typeof(_mask))__BF_FIELD_CHECK_CONST(_mask, 0ULL, _val) +	\
> +	 (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)))
> +
>  /**
>   * FIELD_GET() - extract a bitfield element
>   * @_mask: shifted mask defining the field's length and position
> @@ -150,4 +173,15 @@ __MAKE_OP(64)
>  #undef __MAKE_OP
>  #undef ____MAKE_OP
>
> +#define __encode_bits(w, v, field)	__FIELD_PREP((u##w)(field), v)
> +#define __u16_encode_bits(v, field)	__encode_bits(16, v, field)
> +#define __le16_encode_bits(v, field)	cpu_to_le16(__encode_bits(16, v, field))
> +#define __be16_encode_bits(v, field)	cpu_to_be16(__encode_bits(16, v, field))
> +#define __u32_encode_bits(v, field)	__encode_bits(32, v, field)
> +#define __le32_encode_bits(v, field)	cpu_to_le32(__encode_bits(32, v, field))
> +#define __be32_encode_bits(v, field)	cpu_to_be32(__encode_bits(32, v, field))
> +#define __u64_encode_bits(v, field)	__encode_bits(64, v, field)
> +#define __le64_encode_bits(v, field)	cpu_to_le64(__encode_bits(64, v, field))
> +#define __be64_encode_bits(v, field)	cpu_to_be64(__encode_bits(64, v, field))
> +
>  #endif
>

Thanks again,
John