Re: [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Yury Norov]

Hi Lucas, all!

(Thanks, Andy, for pointing to this thread.)

On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> masks for fixed-width types and also the corresponding BIT_U32(),
> BIT_U16() and BIT_U8().

Can you split BIT() and GENMASK() material to separate patches?

> All of those depend on a new "U" suffix added to the integer constant.
> Due to naming clashes it's better to call the macro U32. Since C doesn't
> have a proper suffix for short and char types, the U16 and U18 variants
> just use U32 with one additional check in the BIT_* macros to make
> sure the compiler gives an error when the those types overflow.

I feel like I don't understand the sentence...

> The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(),
> as otherwise they would allow an invalid bit to be passed. Hence
> implement them in include/linux/bits.h rather than together with
> the other BIT* variants.

I don't think it's a good way to go because BIT() belongs to a more basic
level than GENMASK(). Not mentioning possible header dependency issues.
If you need to test against tighter numeric region, I'd suggest to
do the same trick as  GENMASK_INPUT_CHECK() does, but in uapi/linux/const.h
directly. Something like:
        #define _U8(x)		(CONST_GT(U8_MAX, x) + _AC(x, U))

> The following test file is is used to test this:
> 
> 	$ cat mask.c
> 	#include <linux/types.h>
> 	#include <linux/bits.h>
> 
> 	static const u32 a = GENMASK_U32(31, 0);
> 	static const u16 b = GENMASK_U16(15, 0);
> 	static const u8 c = GENMASK_U8(7, 0);
> 	static const u32 x = BIT_U32(31);
> 	static const u16 y = BIT_U16(15);
> 	static const u8 z = BIT_U8(7);
> 
> 	#if FAIL
> 	static const u32 a2 = GENMASK_U32(32, 0);
> 	static const u16 b2 = GENMASK_U16(16, 0);
> 	static const u8 c2 = GENMASK_U8(8, 0);
> 	static const u32 x2 = BIT_U32(32);
> 	static const u16 y2 = BIT_U16(16);
> 	static const u8 z2 = BIT_U8(8);
> 	#endif
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> ---
>  include/linux/bits.h       | 22 ++++++++++++++++++++++
>  include/uapi/linux/const.h |  2 ++
>  include/vdso/const.h       |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 7c0cf5031abe..ff4786c99b8c 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -42,4 +42,26 @@
>  #define GENMASK_ULL(h, l) \
>  	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>  
> +#define __GENMASK_U32(h, l) \
> +	(((~U32(0)) - (U32(1) << (l)) + 1) & \
> +	 (~U32(0) >> (32 - 1 - (h))))
> +#define GENMASK_U32(h, l) \
> +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(h, l))
> +
> +#define __GENMASK_U16(h, l) \
> +	((U32(0xffff) - (U32(1) << (l)) + 1) & \
> +	 (U32(0xffff) >> (16 - 1 - (h))))
> +#define GENMASK_U16(h, l) \
> +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U16(h, l))
> +
> +#define __GENMASK_U8(h, l) \
> +	(((U32(0xff)) - (U32(1) << (l)) + 1) & \
> +	 (U32(0xff) >> (8 - 1 - (h))))
> +#define GENMASK_U8(h, l) \
> +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U8(h, l))

[...]

I see nothing wrong with fixed-wight versions of GENMASK if it helps
people to write safer code. Can you please in commit message mention
the exact patch(es) that added a bug related to GENMASK() misuse? It
would be easier to advocate the purpose of new API with that in mind.

Regarding implementation - we should avoid copy-pasting in cases
like this. Below is the patch that I boot-tested for x86_64 and
compile-tested for arm64.

It looks less opencoded, and maybe Andy will be less skeptical about
this approach because of less maintenance burden. Please take it if
you like for v2.

Thanks,
Yury