On Fri, 28 Oct 2022, Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> wrote: > Resend, because some content was accidentally omitted from the previous > reply. > Please ignore the previous email. > > Hi all, > > I should have written the original commit message more accurately, but > it seems that it was written inaccurately. > > If the FIELD_PREP macro is expanded, the following macros are used. > > #define FIELD_PREP(_mask, _val) \ > ({ \ > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > > #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ > ({ \ > 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))); \ > }) > > Among them, a build error is generated by the lower part of the > __BF_FIELD_CHECK() macro. > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ > __bf_cast_unsigned(_reg, ~0ull), \ > _pfx "type of reg too small for mask"); \ > > > Here, if you apply an argument to this macro, it will look like the > following. > > __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull) > > The result is always false because an unsigned int value of type > field_msk is not always greater than the maximum value of unsigned long > long . > So, a build error occurs due to the following part of the clang compiler > option. > > [-Werror,-Wtautological-constant-out-of-range-compare] > > You can simply override this warning in Clang by adding the build option > below, but this seems like a bad attempt > > i915/Makefile > CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare > > The easiest way to solve this is to use a constant value, not a > variable, as an argument to FIELD_PREP. > > And since the REG_FIELD_PREP() macro suggested by Jani requires a const > expression as the first argument, it cannot be changed with this macro > alone in the existing code, it must be changed to input a constant value > as shown below. We've added REG_FIELD_PREP() precisely to avoid the problems with the types and ranges, as we want it to operate on u32. It also uses __is_constexpr() to avoid dependencies on compiler implementation and optimizations. Please use REG_FIELD_PREP() and a constant value. Maybe rethink the interface if needed. BR, Jani. > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c > b/drivers/gpu/drm/i915/i915_hwmon.c > index 08c921421a5f..abb3a194c548 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, > i915_reg_t rgadr, > > static void > hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > - const u32 field_msk, int nshift, > + int nshift, > unsigned int scale_factor, long lval) > { > u32 nval; > @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, > i915_reg_t rgadr, > /* Computation in 64-bits to avoid overflow. Round to nearest. */ > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > > - bits_to_clear = field_msk; > - bits_to_set = REG_FIELD_PREP(field_msk, nval); > + bits_to_clear = PKG_PWR_LIM_1; > + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval); > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > bits_to_clear, bits_to_set); > @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, > int chan, long val) > case hwmon_power_max: > hwm_field_scale_and_write(ddat, > hwmon->rg.pkg_rapl_limit, > - PKG_PWR_LIM_1, > hwmon->scl_shift_power, > SF_POWER, val); > return 0; > > > > In addition, if there is no build problem regardless of the size of the > type as the first argument in FIELD_PREP, it is possible through the > following modification. > (Since this modification modifies include/linux/bitfield.h , I will send > it as a separate patch. > ) > > However, it seems that we need to have Jani's confirm whether it is okay > to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32 > return type in i915. > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index c9be1657f03d..6e96799b6f38 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -9,7 +9,7 @@ > > #include <linux/build_bug.h> > #include <asm/byteorder.h> > - > +#include <linux/overflow.h> > /* > * Bitfield access macros > * > @@ -69,7 +69,7 @@ > ~((_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), \ > + __bf_cast_unsigned(_reg, > type_max(__unsigned_scalar_typeof(_reg))), \ > _pfx "type of reg too small for mask"); \ > __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > (1ULL << __bf_shf(_mask))); \ > @@ -84,7 +84,7 @@ > */ > #define FIELD_MAX(_mask) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \ > + __BF_FIELD_CHECK(_mask, > type_min(__unsigned_scalar_typeof(_mask)), > type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \ > (typeof(_mask))((_mask) >> __bf_shf(_mask)); \ > }) > > @@ -97,7 +97,7 @@ > */ > #define FIELD_FIT(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ > + __BF_FIELD_CHECK(_mask, > type_min(__unsigned_scalar_typeof(_mask)), > type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \ > !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ > }) > > @@ -111,7 +111,7 @@ > */ > #define FIELD_PREP(_mask, _val) > \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > + __BF_FIELD_CHECK(_mask, > type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > @@ -125,7 +125,7 @@ > */ > #define FIELD_GET(_mask, _reg) \ > ({ \ > - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \ > + __BF_FIELD_CHECK(_mask, _reg, > type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \ > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > }) > > > Br, > > G.G. > > On 10/27/22 9:32 PM, Dixit, Ashutosh wrote: >> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote: >>> >> >> Hi Nick, >> >>> Thanks, I can repro now. >>> >>> I haven't detangled the macro soup, but I noticed: >>> >>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the >>> following comment: >>> 18 * Mask must be a compilation time constant. >> >> I had comments about this here: >> >> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@xxxxxxxxx/ >> >> The relevant part being: >> >> ---- {quote} ---- >>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK' >>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ >> >> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull >> also occurs here): >> >> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ >> __bf_cast_unsigned(_reg, ~0ull), \ >> _pfx "type of reg too small for mask"); \ >> >> So it goes through previous checks including the "mask is not constant" >> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated >> after most optimizations have run" so by that time both compilers (gcc and >> clang) have figured out that even though _mask is coming in as function >> argument it is really the constant below: >> >> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0) >> >> But it is not clear why clang chokes on this "type of reg too small for >> mask" check (and gcc doesn't) since everything is u32. >> ---- {end quote} ---- >> >>> >>> 2. hwm_field_scale_and_write only has one callsite. >>> >>> The following patch works: >> >> If we need to fix it at our end yes we can come up with one of these >> patches. But we were hoping someone from clang/llvm can comment about the >> "type of reg too small for mask" stuff. If this is something which needs to >> be fixed in clang/llvm we probably don't want to hide the issue. >> >>> >>> ``` >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c >>> b/drivers/gpu/drm/i915/i915_hwmon.c >>> index 9e9781493025..6ac29d90b92a 100644 >>> --- a/drivers/gpu/drm/i915/i915_hwmon.c >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, >>> i915_reg_t rgadr, >>> >>> static void >>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, >>> - u32 field_msk, int nshift, >>> + int nshift, >>> unsigned int scale_factor, long lval) >>> { >>> u32 nval; >>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata >>> *ddat, i915_reg_t rgadr, >>> /* Computation in 64-bits to avoid overflow. Round to nearest. */ >>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); >>> >>> - bits_to_clear = field_msk; >>> - bits_to_set = FIELD_PREP(field_msk, nval); >>> + bits_to_clear = PKG_PWR_LIM_1; >>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval); >>> >>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, >>> bits_to_clear, bits_to_set); >>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 >>> attr, int chan, long val) >>> case hwmon_power_max: >>> hwm_field_scale_and_write(ddat, >>> hwmon->rg.pkg_rapl_limit, >>> - PKG_PWR_LIM_1, >>> hwmon->scl_shift_power, >>> SF_POWER, val); >>> return 0; >>> ``` >>> Though I'm not sure if you're planning to add further callsites of >>> hwm_field_scale_and_write with different field_masks? >> >> I have reasons for keeping it this way, it's there in the link above if you >> are interested. >> >>> >>> Alternatively, (without the above diff), >>> >>> ``` >>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h >>> index c9be1657f03d..6f40f12bcf89 100644 >>> --- a/include/linux/bitfield.h >>> +++ b/include/linux/bitfield.h >>> @@ -8,6 +8,7 @@ >>> #define _LINUX_BITFIELD_H >>> >>> #include <linux/build_bug.h> >>> +#include <linux/const.h> >>> #include <asm/byteorder.h> >>> >>> /* >>> @@ -62,7 +63,7 @@ >>> >>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ >>> ({ \ >>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ >>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \ >>> _pfx "mask is not constant"); \ >>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ >>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ >>> ``` >>> will produce: >>> error: call to __compiletime_assert_407 declared with 'error' >>> attribute: FIELD_PREP: mask is not constant >>> >>> I haven't tested if that change is also feasible (on top of fixing >>> this specific instance), but I think it might help avoid more of these >>> subtleties wrt. __builtin_constant_p that depende heavily on compiler, >>> compiler version, optimization level. >> >> Not disagreeing, can do something here if needed. >> >> Thanks. >> -- >> Ashutosh -- Jani Nikula, Intel Open Source Graphics Center