The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
(long) long" _mask types. For constant masks, that condition is usually
met, as GENMASK() yields an UL value. The few places where the
constant mask is stored in an intermediate variable were fixed by
changing the variable type to u64 (see e.g. [1] and [2]).
However, for non-constant masks, smaller unsigned types should be valid,
too, but currently lead to "result of comparison of constant
18446744073709551615 with expression of type ... is always
false"-warnings with clang and W=1.
Hence refactor the __BF_FIELD_CHECK() helper, and factor out
__FIELD_{GET,PREP}(). The later lack the single problematic check, but
are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
used in the fully non-const variants later.
[1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
HWM_FIELD_PREP()")
[2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
warnings from clang")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v6:
- New.
Alternatively, FIELD_{GET,PREP}() could be duplicated, with the second
parameter of __BF_FIELD_CHECK() changed from "0ULL" resp. "_reg" to
"_mask":
#define __FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, _mask, _val, "__FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __FIELD_GET(_mask, _reg) \
({ \
__BF_FIELD_CHECK(_mask, _mask, 0U, "__FIELD_GET: "); \
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
But I think that is less intuitive, and prevents defining
FIELD_{GET,PREP}() using __FIELD_{GET,PREP}().
---
include/linux/bitfield.h | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 5355f8f806a97974..bf8e0ae4b5b41038 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -60,7 +60,7 @@
#define __bf_cast_unsigned(type, x) ((__unsigned_scalar_typeof(type))(x))
-#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
+#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant"); \
@@ -69,13 +69,33 @@
~((_mask) >> __bf_shf(_mask)) & \
(0 + (_val)) : 0, \
_pfx "value too large for the field"); \
- BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
- __bf_cast_unsigned(_reg, ~0ull), \
- _pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
+#define __BF_FIELD_CHECK_REG(mask, reg, pfx) \
+ BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
+ __bf_cast_unsigned(reg, ~0ull), \
+ pfx "type of reg too small for mask")
+
+#define __BF_FIELD_CHECK(mask, reg, val, pfx) \
+ ({ \
+ __BF_FIELD_CHECK_MASK(mask, val, pfx); \
+ __BF_FIELD_CHECK_REG(mask, reg, pfx); \
+ })
+
+#define __FIELD_PREP(mask, val, pfx) \
+ ({ \
+ __BF_FIELD_CHECK_MASK(mask, val, pfx); \
+ ((typeof(mask))(val) << __bf_shf(mask)) & (mask); \
+ })
+
+#define __FIELD_GET(mask, reg, pfx) \
+ ({ \
+ __BF_FIELD_CHECK_MASK(mask, 0U, pfx); \
+ (typeof(mask))(((reg) & (mask)) >> __bf_shf(mask)); \
+ })
+
/**
* FIELD_MAX() - produce the maximum value representable by a field
* @_mask: shifted mask defining the field's length and position
@@ -112,8 +132,8 @@
*/
#define FIELD_PREP(_mask, _val) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
- ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
+ __BF_FIELD_CHECK_REG(_mask, 0ULL, "FIELD_PREP: "); \
+ __FIELD_PREP(_mask, _val, "FIELD_PREP: "); \
})
#define __BF_CHECK_POW2(n) BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
@@ -152,8 +172,8 @@
*/
#define FIELD_GET(_mask, _reg) \
({ \
- __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
- (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
+ __BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: "); \
+ __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
})
/**
--
2.43.0On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> (long) long" _mask types. For constant masks, that condition is usually
> met, as GENMASK() yields an UL value. The few places where the
> constant mask is stored in an intermediate variable were fixed by
> changing the variable type to u64 (see e.g. [1] and [2]).
>
> However, for non-constant masks, smaller unsigned types should be valid,
> too, but currently lead to "result of comparison of constant
> 18446744073709551615 with expression of type ... is always
> false"-warnings with clang and W=1.
>
> Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> used in the fully non-const variants later.
>
> [1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
> HWM_FIELD_PREP()")
> [2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
> warnings from clang")
Also can be made as
Link: https://git.kernel.org/torvalds/c/5c667d5a5a3ec166 [1]
The positive effect that one may click that on Git Web.
Ideally, of course, would be an additional parses on Git Web kernel.org uses to
parse that standard "commit ...()" notation to add the respective HREF link.
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
...
> + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> + __bf_cast_unsigned(reg, ~0ull), \
> + pfx "type of reg too small for mask")
Perhaps we may convert this (and others?) to static_assert():s at some point?
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 06, 2025 at 04:44:31PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > (long) long" _mask types. For constant masks, that condition is usually
> > met, as GENMASK() yields an UL value. The few places where the
> > constant mask is stored in an intermediate variable were fixed by
> > changing the variable type to u64 (see e.g. [1] and [2]).
> >
> > However, for non-constant masks, smaller unsigned types should be valid,
> > too, but currently lead to "result of comparison of constant
> > 18446744073709551615 with expression of type ... is always
> > false"-warnings with clang and W=1.
> >
> > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > used in the fully non-const variants later.
> >
> > [1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
> > HWM_FIELD_PREP()")
> > [2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
> > warnings from clang")
>
>
> Also can be made as
>
> Link: https://git.kernel.org/torvalds/c/5c667d5a5a3ec166 [1]
>
> The positive effect that one may click that on Git Web.
> Ideally, of course, would be an additional parses on Git Web kernel.org uses to
> parse that standard "commit ...()" notation to add the respective HREF link.
Those flying over Atlantic or cruising cross Caribbean would disagree. :)
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> ...
>
> > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > + __bf_cast_unsigned(reg, ~0ull), \
> > + pfx "type of reg too small for mask")
>
> Perhaps we may convert this (and others?) to static_assert():s at some point?
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
On Thu, Nov 06, 2025 at 11:01:39AM -0500, Yury Norov wrote:
> On Thu, Nov 06, 2025 at 04:44:31PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > (long) long" _mask types. For constant masks, that condition is usually
> > > met, as GENMASK() yields an UL value. The few places where the
> > > constant mask is stored in an intermediate variable were fixed by
> > > changing the variable type to u64 (see e.g. [1] and [2]).
> > >
> > > However, for non-constant masks, smaller unsigned types should be valid,
> > > too, but currently lead to "result of comparison of constant
> > > 18446744073709551615 with expression of type ... is always
> > > false"-warnings with clang and W=1.
> > >
> > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > used in the fully non-const variants later.
> > >
> > > [1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
> > > HWM_FIELD_PREP()")
> > > [2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
> > > warnings from clang")
> >
> >
> > Also can be made as
> >
> > Link: https://git.kernel.org/torvalds/c/5c667d5a5a3ec166 [1]
> >
> > The positive effect that one may click that on Git Web.
> > Ideally, of course, would be an additional parses on Git Web kernel.org uses to
> > parse that standard "commit ...()" notation to add the respective HREF link.
>
> Those flying over Atlantic or cruising cross Caribbean would disagree. :)
They won't. The purpose of these Links is described in the above commit message.
Even they read the commit message before clicking.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > (long) long" _mask types. For constant masks, that condition is usually
> > met, as GENMASK() yields an UL value. The few places where the
> > constant mask is stored in an intermediate variable were fixed by
> > changing the variable type to u64 (see e.g. [1] and [2]).
> >
> > However, for non-constant masks, smaller unsigned types should be valid,
> > too, but currently lead to "result of comparison of constant
> > 18446744073709551615 with expression of type ... is always
> > false"-warnings with clang and W=1.
> >
> > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > used in the fully non-const variants later.
> >
> > [1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
> > HWM_FIELD_PREP()")
> > [2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
> > warnings from clang")
>
> Also can be made as
>
> Link: https://git.kernel.org/torvalds/c/5c667d5a5a3ec166 [1]
Nooooh... torvalds might click on it, and complain ;-)
> > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > + __bf_cast_unsigned(reg, ~0ull), \
> > + pfx "type of reg too small for mask")
>
> Perhaps we may convert this (and others?) to static_assert():s at some point?
Nick tried that before, without success:
https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Nov 06, 2025 at 03:49:03PM +0100, Geert Uytterhoeven wrote:
> Hi Andy,
>
> On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > (long) long" _mask types. For constant masks, that condition is usually
> > > met, as GENMASK() yields an UL value. The few places where the
> > > constant mask is stored in an intermediate variable were fixed by
> > > changing the variable type to u64 (see e.g. [1] and [2]).
> > >
> > > However, for non-constant masks, smaller unsigned types should be valid,
> > > too, but currently lead to "result of comparison of constant
> > > 18446744073709551615 with expression of type ... is always
> > > false"-warnings with clang and W=1.
> > >
> > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > used in the fully non-const variants later.
> > >
> > > [1] commit 5c667d5a5a3ec166 ("clk: sp7021: Adjust width of _m in
> > > HWM_FIELD_PREP()")
> > > [2] commit cfd6fb45cfaf46fa ("crypto: ccree - avoid out-of-range
> > > warnings from clang")
> >
> > Also can be made as
> >
> > Link: https://git.kernel.org/torvalds/c/5c667d5a5a3ec166 [1]
>
> Nooooh... torvalds might click on it, and complain ;-)
See my reply to Yury.
> > > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > > + __bf_cast_unsigned(reg, ~0ull), \
> > > + pfx "type of reg too small for mask")
> >
> > Perhaps we may convert this (and others?) to static_assert():s at some point?
>
> Nick tried that before, without success:
> https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
Ah, this is unfortunate.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Thu, 6 Nov 2025 at 17:09, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Thu, Nov 06, 2025 at 03:49:03PM +0100, Geert Uytterhoeven wrote:
> > On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > > (long) long" _mask types. For constant masks, that condition is usually
> > > > met, as GENMASK() yields an UL value. The few places where the
> > > > constant mask is stored in an intermediate variable were fixed by
> > > > changing the variable type to u64 (see e.g. [1] and [2]).
> > > >
> > > > However, for non-constant masks, smaller unsigned types should be valid,
> > > > too, but currently lead to "result of comparison of constant
> > > > 18446744073709551615 with expression of type ... is always
> > > > false"-warnings with clang and W=1.
> > > >
> > > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > > used in the fully non-const variants later.
> > > > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > > > + __bf_cast_unsigned(reg, ~0ull), \
> > > > + pfx "type of reg too small for mask")
> > >
> > > Perhaps we may convert this (and others?) to static_assert():s at some point?
> >
> > Nick tried that before, without success:
> > https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
>
> Ah, this is unfortunate.
Of course, it might be an actual bug in the i915 driver...
The extra checking in field_prep() in case the compiler can
determine that the mask is a constant already found a possible bug
in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
drivers/net/wireless/realtek/rtw89/reg.h:
#define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
#define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
this operation...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Andy,
>
> On Thu, 6 Nov 2025 at 17:09, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Nov 06, 2025 at 03:49:03PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > > > (long) long" _mask types. For constant masks, that condition is usually
> > > > > met, as GENMASK() yields an UL value. The few places where the
> > > > > constant mask is stored in an intermediate variable were fixed by
> > > > > changing the variable type to u64 (see e.g. [1] and [2]).
> > > > >
> > > > > However, for non-constant masks, smaller unsigned types should be valid,
> > > > > too, but currently lead to "result of comparison of constant
> > > > > 18446744073709551615 with expression of type ... is always
> > > > > false"-warnings with clang and W=1.
> > > > >
> > > > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > > > used in the fully non-const variants later.
>
> > > > > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > > > > + __bf_cast_unsigned(reg, ~0ull), \
> > > > > + pfx "type of reg too small for mask")
> > > >
> > > > Perhaps we may convert this (and others?) to static_assert():s at some point?
> > >
> > > Nick tried that before, without success:
> > > https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
> >
> > Ah, this is unfortunate.
>
> Of course, it might be an actual bug in the i915 driver...
>
> The extra checking in field_prep() in case the compiler can
> determine that the mask is a constant already found a possible bug
> in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
>
> rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
>
> drivers/net/wireless/realtek/rtw89/reg.h:
>
> #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
>
> so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> this operation...
The purpose of the statements is to update values excluding bits of
B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
rtw89_write32_mask(). Then, operation looks like
orig = read(reg);
new = (orig & ~mask) | (data & mask);
write(new);
Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
can you find the problem? Please guide us. Thanks.
Ping-Ke
Hi Ping-Ke,
On Fri, 7 Nov 2025 at 02:16, Ping-Ke Shih <pkshih@realtek.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > The extra checking in field_prep() in case the compiler can
> > determine that the mask is a constant already found a possible bug
> > in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
> >
> > rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> >
> > drivers/net/wireless/realtek/rtw89/reg.h:
> >
> > #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> > #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
> >
> > so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> > this operation...
>
> The purpose of the statements is to update values excluding bits of
> B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
> the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
> rtw89_write32_mask(). Then, operation looks like
>
> orig = read(reg);
> new = (orig & ~mask) | (data & mask);
> write(new);
Thanks for your quick confirmation!
So the intention really is to clear bits 22-31, and write the rx_fltr
value to bits 0-15?
if the clearing is not needed, it would be better to use
#define B_AX_RX_FLTR_CFG_MASK GENMASK(15, 0)
If the clearing is needed, I still think it would be better to
change B_AX_RX_FLTR_CFG_MASK, and split the clearing off in a separate
operation, to make it more explicit and obvious for the casual reader.
> Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
> can you find the problem? Please guide us. Thanks.
I still have "[PATCH/RFC 17/17] rtw89: Use bitfield helpers"
https://lore.kernel.org/all/f7b81122f7596fa004188bfae68f25a68c2d2392.1637592133.git.geert+renesas@glider.be/
in my local tree, which started flagging the use of a discontiguous
mask with the improved checking in field_prep().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
Ping-Ke Shih <pkshih@realtek.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Hi Ping-Ke,
> >
> > On Fri, 7 Nov 2025 at 02:16, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > The extra checking in field_prep() in case the compiler can
> > > > determine that the mask is a constant already found a possible bug
> > > > in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
> > > >
> > > > rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> > > >
> > > > drivers/net/wireless/realtek/rtw89/reg.h:
> > > >
> > > > #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> > > > #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
> > > >
> > > > so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> > > > this operation...
> > >
> > > The purpose of the statements is to update values excluding bits of
> > > B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
> > > the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
> > > rtw89_write32_mask(). Then, operation looks like
> > >
> > > orig = read(reg);
> > > new = (orig & ~mask) | (data & mask);
> > > write(new);
> >
> > Thanks for your quick confirmation!
> > So the intention really is to clear bits 22-31, and write the rx_fltr
> > value to bits 0-15?
> >
> > if the clearing is not needed, it would be better to use
> > #define B_AX_RX_FLTR_CFG_MASK GENMASK(15, 0)
>
> But it should be
> #define B_AX_RX_FLTR_CFG_MASK (GENMASK(31, 22) | GENMASK(15, 0))
>
> Originally (with bug) we just backup rx_fltr and write whole 32-bits back,
> but it's incorrect to modify GENMASK(21, 16) which is written by another
> code.
>
> One way is to implement a special function to replace
> rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> Such as
> rtw89_write_rx_flter(rtwdev, rtwdev->hal.rx_fltr)
> {
> orig = read(reg);
> new = (orig & ~mask) | (data & mask);
> write(new);
> }
>
> Another way is that I add value of B_AX_RX_MPDU_MAX_LEN_MASK into
> rtwdev->hal.rx_fltr. Then, just write whole 32-bit, no need mask.
>
> >
> > If the clearing is needed, I still think it would be better to
> > change B_AX_RX_FLTR_CFG_MASK, and split the clearing off in a separate
> > operation, to make it more explicit and obvious for the casual reader.
I missed this paragraph last week, but that is like my thought.
Then I spin a RFT [1]. Please try if it can work with your changes.
[1] https://lore.kernel.org/linux-wireless/20251110023707.6272-1-pkshih@realtek.com/T/#u
> >
> > > Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
> > > can you find the problem? Please guide us. Thanks.
> >
> > I still have "[PATCH/RFC 17/17] rtw89: Use bitfield helpers"
> >
> https://lore.kernel.org/all/f7b81122f7596fa004188bfae68f25a68c2d2392.1637592133.git.geert+renesas@glid
> > er.be/
> > in my local tree, which started flagging the use of a discontiguous
> > mask with the improved checking in field_prep().
>
> Got it. You are doing "Non-const bitfield helper conversions".
>
> Ping-Ke
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ping-Ke,
>
> On Fri, 7 Nov 2025 at 02:16, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > The extra checking in field_prep() in case the compiler can
> > > determine that the mask is a constant already found a possible bug
> > > in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
> > >
> > > rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> > >
> > > drivers/net/wireless/realtek/rtw89/reg.h:
> > >
> > > #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> > > #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
> > >
> > > so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> > > this operation...
> >
> > The purpose of the statements is to update values excluding bits of
> > B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
> > the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
> > rtw89_write32_mask(). Then, operation looks like
> >
> > orig = read(reg);
> > new = (orig & ~mask) | (data & mask);
> > write(new);
>
> Thanks for your quick confirmation!
> So the intention really is to clear bits 22-31, and write the rx_fltr
> value to bits 0-15?
>
> if the clearing is not needed, it would be better to use
> #define B_AX_RX_FLTR_CFG_MASK GENMASK(15, 0)
But it should be
#define B_AX_RX_FLTR_CFG_MASK (GENMASK(31, 22) | GENMASK(15, 0))
Originally (with bug) we just backup rx_fltr and write whole 32-bits back,
but it's incorrect to modify GENMASK(21, 16) which is written by another
code.
One way is to implement a special function to replace
rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
Such as
rtw89_write_rx_flter(rtwdev, rtwdev->hal.rx_fltr)
{
orig = read(reg);
new = (orig & ~mask) | (data & mask);
write(new);
}
Another way is that I add value of B_AX_RX_MPDU_MAX_LEN_MASK into
rtwdev->hal.rx_fltr. Then, just write whole 32-bit, no need mask.
>
> If the clearing is needed, I still think it would be better to
> change B_AX_RX_FLTR_CFG_MASK, and split the clearing off in a separate
> operation, to make it more explicit and obvious for the casual reader.
>
> > Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
> > can you find the problem? Please guide us. Thanks.
>
> I still have "[PATCH/RFC 17/17] rtw89: Use bitfield helpers"
> https://lore.kernel.org/all/f7b81122f7596fa004188bfae68f25a68c2d2392.1637592133.git.geert+renesas@glid
> er.be/
> in my local tree, which started flagging the use of a discontiguous
> mask with the improved checking in field_prep().
Got it. You are doing "Non-const bitfield helper conversions".
Ping-Ke
On Fri, Nov 7, 2025 at 3:16 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
>
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, 6 Nov 2025 at 17:09, Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Nov 06, 2025 at 03:49:03PM +0100, Geert Uytterhoeven wrote:
> > > > On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
> > > > <andriy.shevchenko@intel.com> wrote:
> > > > > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > > > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > > > > (long) long" _mask types. For constant masks, that condition is usually
> > > > > > met, as GENMASK() yields an UL value. The few places where the
> > > > > > constant mask is stored in an intermediate variable were fixed by
> > > > > > changing the variable type to u64 (see e.g. [1] and [2]).
> > > > > >
> > > > > > However, for non-constant masks, smaller unsigned types should be valid,
> > > > > > too, but currently lead to "result of comparison of constant
> > > > > > 18446744073709551615 with expression of type ... is always
> > > > > > false"-warnings with clang and W=1.
> > > > > >
> > > > > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > > > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > > > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > > > > used in the fully non-const variants later.
> >
> > > > > > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > > > > > + __bf_cast_unsigned(reg, ~0ull), \
> > > > > > + pfx "type of reg too small for mask")
> > > > >
> > > > > Perhaps we may convert this (and others?) to static_assert():s at some point?
> > > >
> > > > Nick tried that before, without success:
> > > > https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
> > >
> > > Ah, this is unfortunate.
> >
> > Of course, it might be an actual bug in the i915 driver...
> >
> > The extra checking in field_prep() in case the compiler can
> > determine that the mask is a constant already found a possible bug
> > in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
> >
> > rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> >
> > drivers/net/wireless/realtek/rtw89/reg.h:
> >
> > #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> > #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
> >
> > so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> > this operation...
>
> The purpose of the statements is to update values excluding bits of
> B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
> the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
> rtw89_write32_mask(). Then, operation looks like
>
> orig = read(reg);
> new = (orig & ~mask) | (data & mask);
> write(new);
>
> Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
> can you find the problem? Please guide us. Thanks.
Isn't FIELD_MODIFY() what you want to use?
--
With Best Regards,
Andy Shevchenko
Hi Ady,
On Fri, 7 Nov 2025 at 09:00, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Nov 7, 2025 at 3:16 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, 6 Nov 2025 at 17:09, Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Thu, Nov 06, 2025 at 03:49:03PM +0100, Geert Uytterhoeven wrote:
> > > > > On Thu, 6 Nov 2025 at 15:44, Andy Shevchenko
> > > > > <andriy.shevchenko@intel.com> wrote:
> > > > > > On Thu, Nov 06, 2025 at 02:34:00PM +0100, Geert Uytterhoeven wrote:
> > > > > > > The BUILD_BUG_ON_MSG() check against "~0ull" works only with "unsigned
> > > > > > > (long) long" _mask types. For constant masks, that condition is usually
> > > > > > > met, as GENMASK() yields an UL value. The few places where the
> > > > > > > constant mask is stored in an intermediate variable were fixed by
> > > > > > > changing the variable type to u64 (see e.g. [1] and [2]).
> > > > > > >
> > > > > > > However, for non-constant masks, smaller unsigned types should be valid,
> > > > > > > too, but currently lead to "result of comparison of constant
> > > > > > > 18446744073709551615 with expression of type ... is always
> > > > > > > false"-warnings with clang and W=1.
> > > > > > >
> > > > > > > Hence refactor the __BF_FIELD_CHECK() helper, and factor out
> > > > > > > __FIELD_{GET,PREP}(). The later lack the single problematic check, but
> > > > > > > are otherwise identical to FIELD_{GET,PREP}(), and are intended to be
> > > > > > > used in the fully non-const variants later.
> > >
> > > > > > > + BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) > \
> > > > > > > + __bf_cast_unsigned(reg, ~0ull), \
> > > > > > > + pfx "type of reg too small for mask")
> > > > > >
> > > > > > Perhaps we may convert this (and others?) to static_assert():s at some point?
> > > > >
> > > > > Nick tried that before, without success:
> > > > > https://lore.kernel.org/all/CAKwvOdm_prtk1UQNJQGidZm44Lk582S3p=of0y46+rVjnSgXJg@mail.gmail.com
> > > >
> > > > Ah, this is unfortunate.
> > >
> > > Of course, it might be an actual bug in the i915 driver...
> > >
> > > The extra checking in field_prep() in case the compiler can
> > > determine that the mask is a constant already found a possible bug
> > > in drivers/net/wireless/realtek/rtw89/core.c:rtw89_roc_end():
> > >
> > > rtw89_write32_mask(rtwdev, reg, B_AX_RX_FLTR_CFG_MASK, rtwdev->hal.rx_fltr);
> > >
> > > drivers/net/wireless/realtek/rtw89/reg.h:
> > >
> > > #define B_AX_RX_MPDU_MAX_LEN_MASK GENMASK(21, 16)
> > > #define B_AX_RX_FLTR_CFG_MASK ((u32)~B_AX_RX_MPDU_MAX_LEN_MASK)
> > >
> > > so it looks like B_AX_RX_FLTR_CFG_MASK is not the proper mask for
> > > this operation...
> >
> > The purpose of the statements is to update values excluding bits of
> > B_AX_RX_MPDU_MAX_LEN_MASK. The use of B_AX_RX_FLTR_CFG_MASK is tricky, but
> > the operation is correct because bit 0 is set, so __ffs(mask) returns 0 in
> > rtw89_write32_mask(). Then, operation looks like
> >
> > orig = read(reg);
> > new = (orig & ~mask) | (data & mask);
> > write(new);
> >
> > Since we don't use FIELD_{GET,PREP} macros with B_AX_RX_FLTR_CFG_MASK, how
> > can you find the problem? Please guide us. Thanks.
>
> Isn't FIELD_MODIFY() what you want to use?
FIELD_MODIFY() is a rather recent addition.
That is also the reason why I didn't add a non-const field_modify() yet
(I didn't want to risk delaying this series even more ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
© 2016 - 2025 Red Hat, Inc.