| Message ID | 20250306-fixed-type-genmasks-v5-2-b443e9dcba63@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bits: Fixed-type GENMASK()/BIT() | expand |
On Thu, Mar 06, 2025 at 08:29:53PM +0900, Vincent Mailhol via B4 Relay wrote: > From: Yury Norov <yury.norov@gmail.com> > > Add GENMASK_TYPE() which generalizes __GENMASK() to support different > types, and implement fixed-types versions of GENMASK() based on it. > The fixed-type version allows more strict checks to the min/max values > accepted, which is useful for defining registers like implemented by > i915 and xe drivers with their REG_GENMASK*() macros. > > The strict checks rely on shift-count-overflow compiler check to fail > the build if a number outside of the range allowed is passed. > Example: > > #define FOO_MASK GENMASK_U32(33, 4) > > will generate a warning like: > > include/linux/bits.h:51:27: error: right shift count >= width of type [-Werror=shift-count-overflow] > 51 | type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h))))) > | ^~ Code LGTM, but just to be sure: you prepared your series using histogram diff algo, right?
On 06/03/2025 à 22:08, Andy Shevchenko wrote: > On Thu, Mar 06, 2025 at 08:29:53PM +0900, Vincent Mailhol via B4 Relay wrote: >> From: Yury Norov <yury.norov@gmail.com> >> >> Add GENMASK_TYPE() which generalizes __GENMASK() to support different >> types, and implement fixed-types versions of GENMASK() based on it. >> The fixed-type version allows more strict checks to the min/max values >> accepted, which is useful for defining registers like implemented by >> i915 and xe drivers with their REG_GENMASK*() macros. >> >> The strict checks rely on shift-count-overflow compiler check to fail >> the build if a number outside of the range allowed is passed. >> Example: >> >> #define FOO_MASK GENMASK_U32(33, 4) >> >> will generate a warning like: >> >> include/linux/bits.h:51:27: error: right shift count >= width of type [-Werror=shift-count-overflow] >> 51 | type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h))))) >> | ^~ > > Code LGTM Does this mean I get your Reviewed-by tag? Or will you wait the v6 to formally give it? > but just to be sure: you prepared your series using histogram > diff algo, right? No, I never used the histogram diff. My git config is extremely boring. Mostly vanilla. I remember that Linus even commented on this: https://lore.kernel.org/all/CAHk-=wiUxm-NZ1si8dXWVTTJ9n3c+1SRTC0V+Lk7hOE4bDVwJQ@mail.gmail.com/ But he made it clear this was *not* a requirement, so I just left the diff algorithm to the default. Or did I miss any communication that contributors should now use histogram diff? Regardless, I do not mind activating it. I just did a: git config diff.algorithm histogram The v6 will have histogram diffs. Yours sincerely, Vincent Mailhol
On Fri, Mar 07, 2025 at 01:08:01AM +0900, Vincent Mailhol wrote: > On 06/03/2025 à 22:08, Andy Shevchenko wrote: > > On Thu, Mar 06, 2025 at 08:29:53PM +0900, Vincent Mailhol via B4 Relay wrote: ... > Does this mean I get your Reviewed-by tag? Or will you wait the v6 to > formally give it? I'll wait for v6. > > but just to be sure: you prepared your series using histogram > > diff algo, right? > > No, I never used the histogram diff. My git config is extremely boring. > Mostly vanilla. > > I remember that Linus even commented on this: > > https://lore.kernel.org/all/CAHk-=wiUxm-NZ1si8dXWVTTJ9n3c+1SRTC0V+Lk7hOE4bDVwJQ@mail.gmail.com/ > > But he made it clear this was *not* a requirement, so I just left the > diff algorithm to the default. Or did I miss any communication that > contributors should now use histogram diff? Use your common sense, i.e. look at the result and evaluate which one is more readable. My guts are telling me that histogram will be slightly better here. > Regardless, I do not mind activating it. I just did a: > > git config diff.algorithm histogram > > The v6 will have histogram diffs.
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -8,7 +8,6 @@ #include <uapi/linux/kernel.h> -#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) #define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) diff --git a/include/linux/bits.h b/include/linux/bits.h index 4819cbe7bd48fbae796fc6005c9f92b1c93a034c..74219521a56e2639ccff7fdc899d6805ee355a0c 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -2,16 +2,15 @@ #ifndef __LINUX_BITS_H #define __LINUX_BITS_H -#include <linux/const.h> #include <vdso/bits.h> #include <uapi/linux/bits.h> -#include <asm/bitsperlong.h> #define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BIT_ULL_MASK(nr) (ULL(1) << ((nr) % BITS_PER_LONG_LONG)) #define BIT_ULL_WORD(nr) ((nr) / BITS_PER_LONG_LONG) #define BITS_PER_BYTE 8 +#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) /* * Create a contiguous bitmask starting at bit position @l and ending at @@ -20,28 +19,44 @@ */ #if !defined(__ASSEMBLY__) +/* + * Missing asm support + * + * GENMASK_U*() depends on BITS_PER_TYPE() which relies on sizeof(), + * something not available in asm. Nethertheless, fixed width integers + * is a C concept. Assembly code can rely on the long and long long + * versions instead. + */ + #include <linux/build_bug.h> #include <linux/compiler.h> +#include <linux/overflow.h> #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) -#define GENMASK(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) -#define GENMASK_ULL(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) - /* - * Missing asm support + * Generate a mask for the specified type @t. Additional checks are made to + * guarantee the value returned fits in that type, relying on + * shift-count-overflow compiler check to detect incompatible arguments. + * For example, all these create build errors or warnings: * - * __GENMASK_U128() depends on _BIT128() which would not work - * in the asm code, as it shifts an 'unsigned __int128' data - * type instead of direct representation of 128 bit constants - * such as long and unsigned long. The fundamental problem is - * that a 128 bit constant will get silently truncated by the - * gcc compiler. + * - GENMASK(15, 20): wrong argument order + * - GENMASK(72, 15): doesn't fit unsigned long + * - GENMASK_U32(33, 15): doesn't fit in a u32 */ -#define GENMASK_U128(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l)) +#define GENMASK_TYPE(t, h, l) \ + ((t)(GENMASK_INPUT_CHECK(h, l) + \ + (type_max(t) << (l) & \ + type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h))))) + +#define GENMASK(h, l) GENMASK_TYPE(unsigned long, h, l) +#define GENMASK_ULL(h, l) GENMASK_TYPE(unsigned long long, h, l) + +#define GENMASK_U8(h, l) GENMASK_TYPE(u8, h, l) +#define GENMASK_U16(h, l) GENMASK_TYPE(u16, h, l) +#define GENMASK_U32(h, l) GENMASK_TYPE(u32, h, l) +#define GENMASK_U64(h, l) GENMASK_TYPE(u64, h, l) +#define GENMASK_U128(h, l) GENMASK_TYPE(u128, h, l) #else /* defined(__ASSEMBLY__) */