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

From: Gwan-gyeong Mun
Date: Fri Oct 28 2022 - 02:49:21 EST

Next message: Li Zetao: "[PATCH v2] mm/mmap: Fix memory leak in mmap_region()"
Previous message: Chao Peng: "Re: [PATCH v9 6/8] KVM: Update lpage info when private/shared memory are mixed"
In reply to: Gwan-gyeong Mun: "Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler"
Next in thread: Jani Nikula: "Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

Next message: Li Zetao: "[PATCH v2] mm/mmap: Fix memory leak in mmap_region()"
Previous message: Chao Peng: "Re: [PATCH v9 6/8] KVM: Update lpage info when private/shared memory are mixed"
In reply to: Gwan-gyeong Mun: "Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler"
Next in thread: Jani Nikula: "Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]