| Message ID | 20221031172655.606165-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/hwmon: Don't use FIELD_PREP | expand |
On Mon, 31 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > FIELD_PREP and REG_FIELD_PREP have checks requiring a compile time constant > mask. When the mask comes in as the argument of a function these checks can > can fail depending on the compiler (gcc vs clang), optimization level, > etc. Use a simpler version of FIELD_PREP which skips these checks. The > checks are not needed because the mask is formed using REG_GENMASK (so is > actually a compile time constant). > > v2: Split REG_FIELD_PREP into a macro with checks and one without and use > the one without checks in i915_hwmon.c (Gwan-gyeong Mun) I frankly think you're solving the wrong problem here. See [1]. BR, Jani. [1] https://lore.kernel.org/r/87leov7yix.fsf@intel.com > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7354 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 2 +- > drivers/gpu/drm/i915/i915_reg_defs.h | 17 +++++++++++------ > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 9e97814930254..ae435b035229a 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -112,7 +112,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > 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_set = __REG_FIELD_PREP(field_msk, nval); > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > bits_to_clear, bits_to_set); > diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h > index f1859046a9c48..dddacc8d48928 100644 > --- a/drivers/gpu/drm/i915/i915_reg_defs.h > +++ b/drivers/gpu/drm/i915/i915_reg_defs.h > @@ -67,12 +67,17 @@ > * > * @return: @__val masked and shifted into the field defined by @__mask. > */ > -#define REG_FIELD_PREP(__mask, __val) \ > - ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) + \ > - BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ > - BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ > - BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ > - BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0)))) > +#define __REG_FIELD_PREP_CHK(__mask, __val) \ > + (BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ > + BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ > + BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ > + BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))) > + > +#define __REG_FIELD_PREP(__mask, __val) \ > + ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)))) > + > +#define REG_FIELD_PREP(__mask, __val) \ > + (__REG_FIELD_PREP(__mask, __val) + __REG_FIELD_PREP_CHK(__mask, __val)) > > /** > * REG_FIELD_GET() - Extract a u32 bitfield value
On Tue, 01 Nov 2022 03:58:13 -0700, Jani Nikula wrote: > > On Mon, 31 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > > FIELD_PREP and REG_FIELD_PREP have checks requiring a compile time constant > > mask. When the mask comes in as the argument of a function these checks can > > can fail depending on the compiler (gcc vs clang), optimization level, > > etc. Use a simpler version of FIELD_PREP which skips these checks. The > > checks are not needed because the mask is formed using REG_GENMASK (so is > > actually a compile time constant). > > > > v2: Split REG_FIELD_PREP into a macro with checks and one without and use > > the one without checks in i915_hwmon.c (Gwan-gyeong Mun) > > I frankly think you're solving the wrong problem here. See [1]. We can consider the sort of refactoring suggested in [1] in the future, right now I thought I'll offer what in my opinion is the correct way to fix the clang compile break incrementally with the current code. But otherwise feel free to go with whatever you think is the correct course of action for this issue. Even if we don't fix the issue the clang guys will (as they have in the past). Thanks. -- Ashutosh > [1] https://lore.kernel.org/r/87leov7yix.fsf@intel.com > > > > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7354 > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 2 +- > > drivers/gpu/drm/i915/i915_reg_defs.h | 17 +++++++++++------ > > 2 files changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 9e97814930254..ae435b035229a 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -112,7 +112,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > > 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_set = __REG_FIELD_PREP(field_msk, nval); > > > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > > bits_to_clear, bits_to_set); > > diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h > > index f1859046a9c48..dddacc8d48928 100644 > > --- a/drivers/gpu/drm/i915/i915_reg_defs.h > > +++ b/drivers/gpu/drm/i915/i915_reg_defs.h > > @@ -67,12 +67,17 @@ > > * > > * @return: @__val masked and shifted into the field defined by @__mask. > > */ > > -#define REG_FIELD_PREP(__mask, __val) \ > > - ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) + \ > > - BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ > > - BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ > > - BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ > > - BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0)))) > > +#define __REG_FIELD_PREP_CHK(__mask, __val) \ > > + (BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ > > + BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ > > + BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ > > + BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))) > > + > > +#define __REG_FIELD_PREP(__mask, __val) \ > > + ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)))) > > + > > +#define REG_FIELD_PREP(__mask, __val) \ > > + (__REG_FIELD_PREP(__mask, __val) + __REG_FIELD_PREP_CHK(__mask, __val)) > > > > /** > > * REG_FIELD_GET() - Extract a u32 bitfield value
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 9e97814930254..ae435b035229a 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -112,7 +112,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, 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_set = __REG_FIELD_PREP(field_msk, nval); hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, bits_to_clear, bits_to_set); diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h index f1859046a9c48..dddacc8d48928 100644 --- a/drivers/gpu/drm/i915/i915_reg_defs.h +++ b/drivers/gpu/drm/i915/i915_reg_defs.h @@ -67,12 +67,17 @@ * * @return: @__val masked and shifted into the field defined by @__mask. */ -#define REG_FIELD_PREP(__mask, __val) \ - ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) + \ - BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ - BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ - BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ - BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0)))) +#define __REG_FIELD_PREP_CHK(__mask, __val) \ + (BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \ + BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \ + BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \ + BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))) + +#define __REG_FIELD_PREP(__mask, __val) \ + ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)))) + +#define REG_FIELD_PREP(__mask, __val) \ + (__REG_FIELD_PREP(__mask, __val) + __REG_FIELD_PREP_CHK(__mask, __val)) /** * REG_FIELD_GET() - Extract a u32 bitfield value
FIELD_PREP and REG_FIELD_PREP have checks requiring a compile time constant mask. When the mask comes in as the argument of a function these checks can can fail depending on the compiler (gcc vs clang), optimization level, etc. Use a simpler version of FIELD_PREP which skips these checks. The checks are not needed because the mask is formed using REG_GENMASK (so is actually a compile time constant). v2: Split REG_FIELD_PREP into a macro with checks and one without and use the one without checks in i915_hwmon.c (Gwan-gyeong Mun) Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7354 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 2 +- drivers/gpu/drm/i915/i915_reg_defs.h | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-)