Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler

Dixit, Ashutosh
2022-10-27T18:33:09Z

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@intel.com/

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