[PATCH] clk: sp7021: Adjust width of _m in HWM_FIELD_PREP()

[Nick Desaulniers]

On Mon, May 1, 2023 at 2:37 PM Nathan Chancellor <nathan at kernel.org> wrote:
>
> On Mon, May 01, 2023 at 02:34:47PM -0700, Nathan Chancellor wrote:
> > When building with clang + W=1, there is a warning around an internal
> > comparison check within the FIELD_PREP() macro, due to a 32-bit variable
> > comparison against ~0ull:
> >
> >   drivers/clk/clk-sp7021.c:316:8: error: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((_m), ...' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >           r0 |= HWM_FIELD_PREP(MASK_SEL_FRA, clk->p[SEL_FRA]);
> >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   drivers/clk/clk-sp7021.c:45:15: note: expanded from macro 'HWM_FIELD_PREP'
> >           (_m << 16) | FIELD_PREP(_m, value);     \
> >                        ^~~~~~~~~~~~~~~~~~~~~
> >   include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> >                   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
> >                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> >                   BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
> >                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> >   note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
> >   include/linux/compiler_types.h:397:22: note: expanded from macro 'compiletime_assert'
> >           _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >           ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   include/linux/compiler_types.h:385:23: note: expanded from macro '_compiletime_assert'
> >           __compiletime_assert(condition, msg, prefix, suffix)
> >           ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   include/linux/compiler_types.h:377:9: note: expanded from macro '__compiletime_assert'
> >                   if (!(condition))                                       \
> >                         ^~~~~~~~~
> >
> > This is expected given the tyoes of the input. Increase the size of the

s/tyoes/types/

Thanks for the patch!

Reviewed-by: Nick Desaulniers <ndesaulniers at google.com>

Nathan, do you think anything can be done in FIELD_PREP or even
__BF_FIELD_CHECK using _Static_assert (or static_assert from
include/linux/build_bug.h) to help elucidate to developers that their
mask is too small?  I guess the line 71 of include/linux/bitfield.h
that is trying to warn via BUILD_BUG_ON is itself triggering a
-Wtautological-constant-out-of-range-compare warning that is perhaps
distracting from what the issue is?

If you're able to reproduce the issue locally still, does this seem to work?
```
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index ebfa12f69501..762a27b5ae15 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -62,15 +62,15 @@

 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)                      \
        ({                                                              \
+               static_assert(__bf_cast_unsigned(_mask, _mask) <=       \
+                             __bf_cast_unsigned(_reg, ~0ull),          \
+                             _pfx "type of reg too small for mask");   \
                BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),          \
                                 _pfx "mask is not constant");          \
                BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");    \
                BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?           \
                                 ~((_mask) >> __bf_shf(_mask)) & (_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))); \
        })

```


> > temporary variable in HWM_FIELD_PREP() to eliminate the warning, which
> > follows the logic of commit cfd6fb45cfaf ("crypto: ccree - avoid
> > out-of-range warnings from clang") for the same reasons.
>
> Gah, I forgot:
>
> Reported-by: kernel test robot <lkp at intel.com>
> Link: https://lore.kernel.org/202303221947.pXP2v4xJ-lkp@intel.com/
>
> > Signed-off-by: Nathan Chancellor <nathan at kernel.org>
> > ---
> >  drivers/clk/clk-sp7021.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-sp7021.c b/drivers/clk/clk-sp7021.c
> > index 8fec14120105..11d22043ddd7 100644
> > --- a/drivers/clk/clk-sp7021.c
> > +++ b/drivers/clk/clk-sp7021.c
> > @@ -41,7 +41,7 @@ enum {
> >  /* HIWORD_MASK FIELD_PREP */
> >  #define HWM_FIELD_PREP(mask, value)          \
> >  ({                                           \
> > -     u32 _m = mask;                          \
> > +     u64 _m = mask;                          \
> >       (_m << 16) | FIELD_PREP(_m, value);     \
> >  })
> >
> >
> > ---
> > base-commit: d54c1fd4a51e8fbc7f9da86b0cd338a4f7cd2bb2
> > change-id: 20230501-sp7021-field_prep-warning-223f17aeea8e
> >
> > Best regards,
> > --
> > Nathan Chancellor <nathan at kernel.org>
> >



-- 
Thanks,
~Nick Desaulniers