| Message ID | 20200916071950.1493-3-gilad@benyossef.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | add optional cache params from DT | expand |
hmm... On Wed, Sep 16, 2020 at 4:48 PM kernel test robot <lkp@intel.com> wrote: > > url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151 > base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master > config: arm64-randconfig-r015-20200916 (attached as .config) > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install arm64 cross compiling tool for clang build > # apt-get install binutils-aarch64-linux-gnu > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > >> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare] > cache_params |= FIELD_PREP(mask, val); > ^~~~~~~~~~~~~~~~~~~~~ > include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP' > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK' > BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG' > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert' > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert' > __compiletime_assert(condition, msg, prefix, suffix) > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert' > if (!(condition)) \ > ^~~~~~~~~ I am unable to understand this warning. It looks like it is complaining about a FIELD_GET sanity check that is always false, which makes sense since we're using a constant. Anyone can enlighten me if I've missed something? Thanks, Gilad
On Thu, Sep 17, 2020 at 12:20 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote: > > hmm... > > On Wed, Sep 16, 2020 at 4:48 PM kernel test robot <lkp@intel.com> wrote: > > > > url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master > > config: arm64-randconfig-r015-20200916 (attached as .config) > > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188) > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # install arm64 cross compiling tool for clang build > > # apt-get install binutils-aarch64-linux-gnu > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <lkp@intel.com> > > > > All warnings (new ones prefixed by >>): > > > > >> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare] > > cache_params |= FIELD_PREP(mask, val); > > ^~~~~~~~~~~~~~~~~~~~~ > > include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP' > > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK' > > BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ > > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG' > > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert' > > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert' > > __compiletime_assert(condition, msg, prefix, suffix) > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert' > > if (!(condition)) \ > > ^~~~~~~~~ > > I am unable to understand this warning. It looks like it is > complaining about a FIELD_GET sanity check that is always false, which > makes sense since we're using a constant. > > Anyone can enlighten me if I've missed something? Looked at some of this code recently. I think it may have an issue for masks where sizeof(mask) < sizeof(unsigned long long). In your code, via 0day bot: 107 u32 cache_params, ace_const, val, mask; ... > 120 cache_params |= FIELD_PREP(mask, val); then in include/linux/bitfield.h, we have: 92 #define FIELD_PREP(_mask, _val) \ 93 ({ \ 94 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ 44 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ ... 52 BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ 53 _pfx "type of reg too small for mask"); \ so the 0ULL in FIELD_PREP is important. In __BF_FIELD_CHECK, the typeof(_reg) is unsigned long long (because 0ULL was passed). So we have a comparison between a u32 and a u64; indeed any u32 can never be greater than a u64 that we know has the value of ULLONG_MAX. I did send a series splitting these up, I wonder if they'd help here: https://lore.kernel.org/lkml/20200708230402.1644819-3-ndesaulniers@google.com/
Hi, On Fri, Sep 18, 2020 at 10:39 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Sep 17, 2020 at 12:20 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote: > > ... > > > > I am unable to understand this warning. It looks like it is > > complaining about a FIELD_GET sanity check that is always false, which > > makes sense since we're using a constant. > > > > Anyone can enlighten me if I've missed something? > > Looked at some of this code recently. I think it may have an issue > for masks where sizeof(mask) < sizeof(unsigned long long). > > In your code, via 0day bot: > > 107 u32 cache_params, ace_const, val, mask; > ... > > 120 cache_params |= FIELD_PREP(mask, val); > > then in include/linux/bitfield.h, we have: > > 92 #define FIELD_PREP(_mask, _val) \ > 93 ({ \ > 94 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > > 44 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ > ... > 52 BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ > 53 _pfx "type of reg too small for mask"); \ > > so the 0ULL in FIELD_PREP is important. In __BF_FIELD_CHECK, the > typeof(_reg) is unsigned long long (because 0ULL was passed). So we > have a comparison between a u32 and a u64; indeed any u32 can never be > greater than a u64 that we know has the value of ULLONG_MAX. > > I did send a series splitting these up, I wonder if they'd help here: > https://lore.kernel.org/lkml/20200708230402.1644819-3-ndesaulniers@google.com/ > -- Thanks! This indeed explains this. It seems there is nothing for me to do about it in the driver code though, as it seems the issue is in the macro and you have already posted a fix for it. Thanks again, Gilad
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c index 6f519d3e896c..db497bc065d4 100644 --- a/drivers/crypto/ccree/cc_driver.c +++ b/drivers/crypto/ccree/cc_driver.c @@ -100,6 +100,71 @@ static const struct of_device_id arm_ccree_dev_of_match[] = { }; MODULE_DEVICE_TABLE(of, arm_ccree_dev_of_match); +static void init_cc_dt_params(struct cc_drvdata *drvdata) +{ + struct device *dev = drvdata_to_dev(drvdata); + struct device_node *np = dev->of_node; + u32 cache_params, ace_const, val, mask; + int rc; + + /* register CC_AXIM_CACHE_PARAMS */ + cache_params = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS)); + dev_dbg(dev, "Cache params previous: 0x%08X\n", cache_params); + + rc = of_property_read_u32(np, "awcache", &val); + if (rc) + val = (drvdata->coherent ? 0xb : 0x2); + + mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE); + cache_params &= ~mask; + cache_params |= FIELD_PREP(mask, val); + + /* write AWCACHE also to AWCACHE_LAST */ + mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE_LAST); + cache_params &= ~mask; + cache_params |= FIELD_PREP(mask, val); + + rc = of_property_read_u32(np, "arcache", &val); + if (rc) + val = (drvdata->coherent ? 0xb : 0x2); + + mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_ARCACHE); + cache_params &= ~mask; + cache_params |= FIELD_PREP(mask, val); + + drvdata->cache_params = cache_params; + + dev_dbg(dev, "Cache params current: 0x%08X\n", cache_params); + + if (drvdata->hw_rev <= CC_HW_REV_710) + return; + + /* register CC_AXIM_ACE_CONST */ + ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST)); + dev_dbg(dev, "ACE-const previous: 0x%08X\n", ace_const); + + rc = of_property_read_u32(np, "ardomain", &val); + ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST)); + if (rc) + val = (drvdata->coherent ? 0x2 : 0x3); + + mask = CC_GENMASK(CC_AXIM_ACE_CONST_ARDOMAIN); + ace_const &= ~mask; + ace_const |= FIELD_PREP(mask, val); + + rc = of_property_read_u32(np, "awdomain", &val); + if (rc) + val = (drvdata->coherent ? 0x2 : 0x3); + + mask = CC_GENMASK(CC_AXIM_ACE_CONST_AWDOMAIN); + ace_const &= ~mask; + ace_const |= FIELD_PREP(mask, val); + + dev_dbg(dev, "ACE-const current: 0x%08X\n", ace_const); + + drvdata->ace_const = ace_const; +} + static u32 cc_read_idr(struct cc_drvdata *drvdata, const u32 *idr_offsets) { int i; @@ -218,9 +283,9 @@ bool cc_wait_for_reset_completion(struct cc_drvdata *drvdata) return false; } -int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe) +int init_cc_regs(struct cc_drvdata *drvdata) { - unsigned int val, cache_params; + unsigned int val; struct device *dev = drvdata_to_dev(drvdata); /* Unmask all AXI interrupt sources AXI_CFG1 register */ @@ -245,19 +310,9 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe) cc_iowrite(drvdata, CC_REG(HOST_IMR), ~val); - cache_params = (drvdata->coherent ? CC_COHERENT_CACHE_PARAMS : 0x0); - - val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS)); - - if (is_probe) - dev_dbg(dev, "Cache params previous: 0x%08X\n", val); - - cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params); - val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS)); - - if (is_probe) - dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n", - val, cache_params); + cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), drvdata->cache_params); + if (drvdata->hw_rev >= CC_HW_REV_712) + cc_iowrite(drvdata, CC_REG(AXIM_ACE_CONST), drvdata->ace_const); return 0; } @@ -445,7 +500,9 @@ static int init_cc_resources(struct platform_device *plat_dev) } dev_dbg(dev, "Registered to IRQ: %d\n", irq); - rc = init_cc_regs(new_drvdata, true); + init_cc_dt_params(new_drvdata); + + rc = init_cc_regs(new_drvdata); if (rc) { dev_err(dev, "init_cc_regs failed\n"); goto post_pm_err; diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h index af77b2020350..cd5a51e8a281 100644 --- a/drivers/crypto/ccree/cc_driver.h +++ b/drivers/crypto/ccree/cc_driver.h @@ -155,6 +155,8 @@ struct cc_drvdata { int std_bodies; bool sec_disabled; u32 comp_mask; + u32 cache_params; + u32 ace_const; }; struct cc_crypto_alg { @@ -205,7 +207,7 @@ static inline void dump_byte_array(const char *name, const u8 *the_array, } bool cc_wait_for_reset_completion(struct cc_drvdata *drvdata); -int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe); +int init_cc_regs(struct cc_drvdata *drvdata); void fini_cc_regs(struct cc_drvdata *drvdata); unsigned int cc_get_default_hash_len(struct cc_drvdata *drvdata); diff --git a/drivers/crypto/ccree/cc_pm.c b/drivers/crypto/ccree/cc_pm.c index 3c65bf070c90..d5421b0c6831 100644 --- a/drivers/crypto/ccree/cc_pm.c +++ b/drivers/crypto/ccree/cc_pm.c @@ -45,7 +45,7 @@ static int cc_pm_resume(struct device *dev) } cc_iowrite(drvdata, CC_REG(HOST_POWER_DOWN_EN), POWER_DOWN_DISABLE); - rc = init_cc_regs(drvdata, false); + rc = init_cc_regs(drvdata); if (rc) { dev_err(dev, "init_cc_regs (%x)\n", rc); return rc;
Add optinal ability to customize DMA transactions cache parameters and ACE bus sharability properties and set new defaults. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> --- drivers/crypto/ccree/cc_driver.c | 89 ++++++++++++++++++++++++++------ drivers/crypto/ccree/cc_driver.h | 4 +- drivers/crypto/ccree/cc_pm.c | 2 +- 3 files changed, 77 insertions(+), 18 deletions(-)