On Thu, Oct 27, 2022 at 9:53 AM Dixit, Ashutosh <ashutosh.dixit@xxxxxxxxx> wrote: > > On Thu, 27 Oct 2022 09:35:24 -0700, Nick Desaulniers wrote: > > > > Hi Nick, > > > On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote: > > > > > > Hi Ashutosh, > > > > > > > But I'd wait to hear from clang/llvm folks first. > > > > > > Yeah! Looking forward to getting some ideas :) > > > > Gwan-gyeong, which tree and set of configs are necessary to reproduce > > the observed warning? > > > > Warnings are treated as errors, so I don't want this breaking our CI. > > The following or equivalent should do it: > > git clone https://anongit.freedesktop.org/git/drm/drm-tip > git checkout drm-tip > > Kernel config: > CONFIG_DRM_I915=m > CONFIG_HWMON=y > > Files: > drivers/gpu/drm/i915/i915_hwmon.c/.h > > Thanks for taking a look. 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. 2. hwm_field_scale_and_write only has one callsite. The following patch works: ``` 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? 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. -- Thanks, ~Nick Desaulniers
