From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 42DDFECAAA1 for ; Thu, 27 Oct 2022 18:33:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 26B9010E00B; Thu, 27 Oct 2022 18:33:24 +0000 (UTC) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 14CAF10E6C6 for ; Thu, 27 Oct 2022 18:33:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666895598; x=1698431598; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=bmQm0hnsktZC10zjzfxP6GEGbTeq9P7+PpzgV6BS8Nk=; b=OjqmOWU7vGfd0W89bk3KF48D6opcBKs89ZgTmUUZWdVTx7YxKOUH3lQ6 pKXbU+d/Rjgc5swmRDOceVolrWM0q7Xf0wXo2tLu2xicrAuFVsziSCjtp z0u1+QJgLDmeKrRoMCJXBe622d/JfnsC1Jzi1lLRqD7bI+i7ByZ2SGE8T Px0qeC3pOYN4TJx5QjiVppgzxrST43sXmtYnmJyT89BTcHkWmcJPK9MPG QKbjUzjQaYz4dF2RNpsUTQq7vOuT5NwoD0p4C6El5udrfuXKxuZjyqIsL HzjrbJoDt30Zzc9haZj6r8cCcpeG9gYTS0XZ6RMLRw/h4zVRYszoY18ph Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10513"; a="288705603" X-IronPort-AV: E=Sophos;i="5.95,218,1661842800"; d="scan'208";a="288705603" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Oct 2022 11:32:56 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10513"; a="610468871" X-IronPort-AV: E=Sophos;i="5.95,218,1661842800"; d="scan'208";a="610468871" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.68.192]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Oct 2022 11:32:54 -0700 Date: Thu, 27 Oct 2022 11:32:54 -0700 Message-ID: <875yg5xgkp.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Nick Desaulniers In-Reply-To: References: <20221024210953.1572998-1-gwan-gyeong.mun@intel.com> <87mt9kppb6.wl-ashutosh.dixit@intel.com> <87ilk7pwrw.wl-ashutosh.dixit@intel.com> <877d0lxl6s.wl-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, llvm@lists.linux.dev, linux-kernel@vger.kernel.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" 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 > +#include > #include > > /* > @@ -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