| Message ID | 20240208074521.577076-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixed-type GENMASK/BIT | expand |
Hi Lucas, looks good, just one idea... ... > +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) > +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) > +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) > +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) considering that BIT defines are always referred to unsigned types, I would just call them #define BIT8 #define BIT16 #define BIT32 #define BIT64 what do you think? Andi
On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote: >Hi Lucas, > >looks good, just one idea... > >... > >> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) >> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) >> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) >> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) > >considering that BIT defines are always referred to unsigned >types, I would just call them > >#define BIT8 >#define BIT16 >#define BIT32 >#define BIT64 > >what do you think? it will clash with defines from other headers and not match the ones for GENMASK so I prefer it the other way. thanks Lucas De Marchi > >Andi
On Thu, 08 Feb 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote: >>Hi Lucas, >> >>looks good, just one idea... >> >>... >> >>> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) >>> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) >>> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) >>> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) >> >>considering that BIT defines are always referred to unsigned >>types, I would just call them >> >>#define BIT8 >>#define BIT16 >>#define BIT32 >>#define BIT64 >> >>what do you think? > > it will clash with defines from other headers and not match the ones for > GENMASK so I prefer it the other way. Agreed.
On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote: > Implement fixed-type BIT() to help drivers add stricter checks, like was > done for GENMASK. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss a v2? Anyways, please bear my reviewed-by from v1 for this patch. Thanks, Yury > --- > include/linux/bits.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/linux/bits.h b/include/linux/bits.h > index bd56f32de44e..811846ce110e 100644 > --- a/include/linux/bits.h > +++ b/include/linux/bits.h > @@ -24,12 +24,16 @@ > #define GENMASK_INPUT_CHECK(h, l) \ > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > __is_constexpr((l) > (h)), (l) > (h), 0))) > +#define BIT_INPUT_CHECK(type, b) \ > + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0)))) > #else > /* > * BUILD_BUG_ON_ZERO is not available in h files included from asm files, > * disable the input check if that is the case. > */ > #define GENMASK_INPUT_CHECK(h, l) 0 > +#define BIT_INPUT_CHECK(type, b) 0 > #endif > > /* > @@ -54,4 +58,17 @@ > #define GENMASK_U32(h, l) __GENMASK(u32, h, l) > #define GENMASK_U64(h, l) __GENMASK(u64, h, l) > > +/* > + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The > + * following examples generate compiler warnings due to shift-count-overflow: > + * > + * - BIT_U8(8) > + * - BIT_U32(-1) > + * - BIT_U32(40) > + */ > +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) > +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) > +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) > +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) > + > #endif /* __LINUX_BITS_H */ > -- > 2.43.0
... > >> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) > >> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) > >> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) > >> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) > > > >considering that BIT defines are always referred to unsigned > >types, I would just call them Except that pretty much as soon as you breath on them the u8 and u16 types get converted to int. If you want them to be an unsigned type then you need to cast them to (unsigned int). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote: >On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote: >> Implement fixed-type BIT() to help drivers add stricter checks, like was >> done for GENMASK. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> Acked-by: Jani Nikula <jani.nikula@intel.com> > >So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss >a v2? Anyways, please bear my reviewed-by from v1 for this patch. Jan 23 was actually the v2 and I missed the subject prefix. My understanding was that you were going to apply this through some bitmap tree, but checking MAINTAINERS now it seems there's no git tree associated. So I will just add your r-b and merge this through drm-xe. thanks Lucas De Marchi > >Thanks, >Yury > >> --- >> include/linux/bits.h | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/include/linux/bits.h b/include/linux/bits.h >> index bd56f32de44e..811846ce110e 100644 >> --- a/include/linux/bits.h >> +++ b/include/linux/bits.h >> @@ -24,12 +24,16 @@ >> #define GENMASK_INPUT_CHECK(h, l) \ >> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ >> __is_constexpr((l) > (h)), (l) > (h), 0))) >> +#define BIT_INPUT_CHECK(type, b) \ >> + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ >> + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0)))) >> #else >> /* >> * BUILD_BUG_ON_ZERO is not available in h files included from asm files, >> * disable the input check if that is the case. >> */ >> #define GENMASK_INPUT_CHECK(h, l) 0 >> +#define BIT_INPUT_CHECK(type, b) 0 >> #endif >> >> /* >> @@ -54,4 +58,17 @@ >> #define GENMASK_U32(h, l) __GENMASK(u32, h, l) >> #define GENMASK_U64(h, l) __GENMASK(u64, h, l) >> >> +/* >> + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The >> + * following examples generate compiler warnings due to shift-count-overflow: >> + * >> + * - BIT_U8(8) >> + * - BIT_U32(-1) >> + * - BIT_U32(40) >> + */ >> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) >> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) >> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) >> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) >> + >> #endif /* __LINUX_BITS_H */ >> -- >> 2.43.0
On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote: > On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote: > > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote: > > > Implement fixed-type BIT() to help drivers add stricter checks, like was > > > done for GENMASK. > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > > > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss > > a v2? Anyways, please bear my reviewed-by from v1 for this patch. > > Jan 23 was actually the v2 and I missed the subject prefix. > > My understanding was that you were going to apply this through some > bitmap tree, but checking MAINTAINERS now it seems there's no git tree > associated. So I will just add your r-b and merge this through > drm-xe. I've got a bitmap-related branch. I can move this series in there if you prefer. At your discretion. https://github.com/norov/linux/tree/bitmap_for_next Thanks, Yury
On Thu, Feb 22, 2024 at 06:51:52AM -0800, Yury Norov wrote: >On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote: >> On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote: >> > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote: >> > > Implement fixed-type BIT() to help drivers add stricter checks, like was >> > > done for GENMASK. >> > > >> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> > > Acked-by: Jani Nikula <jani.nikula@intel.com> >> > >> > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss >> > a v2? Anyways, please bear my reviewed-by from v1 for this patch. >> >> Jan 23 was actually the v2 and I missed the subject prefix. >> >> My understanding was that you were going to apply this through some >> bitmap tree, but checking MAINTAINERS now it seems there's no git tree >> associated. So I will just add your r-b and merge this through >> drm-xe. > >I've got a bitmap-related branch. I can move this series in there if >you prefer. At your discretion. > >https://github.com/norov/linux/tree/bitmap_for_next yeah, sounds good. thanks Lucas De Marchi > >Thanks, >Yury
diff --git a/include/linux/bits.h b/include/linux/bits.h index bd56f32de44e..811846ce110e 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -24,12 +24,16 @@ #define GENMASK_INPUT_CHECK(h, l) \ (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ __is_constexpr((l) > (h)), (l) > (h), 0))) +#define BIT_INPUT_CHECK(type, b) \ + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0)))) #else /* * BUILD_BUG_ON_ZERO is not available in h files included from asm files, * disable the input check if that is the case. */ #define GENMASK_INPUT_CHECK(h, l) 0 +#define BIT_INPUT_CHECK(type, b) 0 #endif /* @@ -54,4 +58,17 @@ #define GENMASK_U32(h, l) __GENMASK(u32, h, l) #define GENMASK_U64(h, l) __GENMASK(u64, h, l) +/* + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The + * following examples generate compiler warnings due to shift-count-overflow: + * + * - BIT_U8(8) + * - BIT_U32(-1) + * - BIT_U32(40) + */ +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) + #endif /* __LINUX_BITS_H */