From: Nathan Chancellor <nathan@kernel.org> To: Dmitry Osipenko <digetx@gmail.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>, kernel test robot <lkp@intel.com>, Thierry Reding <thierry.reding@gmail.com>, Jonathan Hunter <jonathanh@nvidia.com>, Nick Desaulniers <ndesaulniers@google.com>, clang-built-linux@googlegroups.com, kbuild-all@lists.01.org, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org Subject: Re: [PATCH v1 2/2] memory: tegra: Enable compile testing for all drivers Date: Tue, 11 May 2021 10:35:03 -0700 [thread overview] Message-ID: <YJrARxhVD7QM/GPv@archlinux-ax161> (raw) In-Reply-To: <3ab5d50b-4955-7144-5d1d-d44cb0892d65@gmail.com> On Tue, May 11, 2021 at 07:00:34PM +0300, Dmitry Osipenko wrote: > 11.05.2021 18:31, Krzysztof Kozlowski пишет: > ... > ~~~~~~~~~~~~~~~~~~~~~^ > >>>>> drivers/memory/tegra/tegra124-emc.c:802:26: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744071562067985 to 2147483665 [-Wconstant-conversion] > >>> emc_ccfifo_writel(emc, EMC_ZQ_CAL_LONG_CMD_DEV0, EMC_ZQ_CAL); > >>> ~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~ > >>> drivers/memory/tegra/tegra124-emc.c:154:36: note: expanded from macro 'EMC_ZQ_CAL_LONG_CMD_DEV0' > >>> (DRAM_DEV_SEL_0 | EMC_ZQ_CAL_LONG | EMC_ZQ_CAL_CMD) > >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ > >>> 13 warnings generated. > >> > >> This doesn't look like a useful warning from clang, it should see that > >> the constant value itself isn't truncated, hence it should be a problem > >> of clang. Do you think it's okay to ignore this nonsense? > > > > I admit I also do not see the real issue here. The DRAM_DEV_SEL_0 fits > > in u32 and there is no other bitwise arithmetic than just OR, so why > > clang assumes it can have 32 most signifcant bits toggled on? > > > > +Cc Nathan and Nick, > > Maybe you could shed some light here on this warning? > > > > Dmitry, > > In general you should not ignore it because: > > 1. This breaks allyesconfig with clang on powerpc (or it is one of the > > stoppers), > > 2. We might want in some future to build it with clang. > > I meant to ignore it from the perspective of the memory drivers, i.e. it > likely should be fixed in clang and not worked around in the code. Thank > you for pinging the right people. I do not think this is a bug in clang, gcc warns the same (just not here in this case): https://godbolt.org/z/e9GWobMnd DRAM_DEV_SEL_0 and DRAM_DEV_SEL_1 are implicitly signed integers because there is no suffix on the literal 1. DRAM_DEV_SEL_0 is 2 << 30, which can be turned into 1 << 31. That is equal to INT_MAX + 1, which then overflows and becomes INT_MIN (undefined behavior). INT_MIN is then promoted to unsigned long because EMC_ZQ_CAL_LONG and EMC_ZQ_CAL_CMD are unsigned long due to the BIT macro, resulting in the gigantic number that clang reports above. I assume that this driver only runs on hardware where unsigned int is the same size as unsigned long, meaning this problem is merely theoretical? Regardless, defining DRAM_DEV_SEL_{0,1} with the BIT macro fixes the warning for me and should make everything work as expected. diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c index 5699d909abc2..a21ca8e0841a 100644 --- a/drivers/memory/tegra/tegra124-emc.c +++ b/drivers/memory/tegra/tegra124-emc.c @@ -272,8 +272,8 @@ #define EMC_PUTERM_ADJ 0x574 #define DRAM_DEV_SEL_ALL 0 -#define DRAM_DEV_SEL_0 (2 << 30) -#define DRAM_DEV_SEL_1 (1 << 30) +#define DRAM_DEV_SEL_0 BIT(31) +#define DRAM_DEV_SEL_1 BIT(30) #define EMC_CFG_POWER_FEATURES_MASK \ (EMC_CFG_DYN_SREF | EMC_CFG_DRAM_ACPD | EMC_CFG_DRAM_CLKSTOP_SR | \
WARNING: multiple messages have this Message-ID (diff)
From: Nathan Chancellor <nathan@kernel.org> To: kbuild-all@lists.01.org Subject: Re: [PATCH v1 2/2] memory: tegra: Enable compile testing for all drivers Date: Tue, 11 May 2021 10:35:03 -0700 [thread overview] Message-ID: <YJrARxhVD7QM/GPv@archlinux-ax161> (raw) In-Reply-To: <3ab5d50b-4955-7144-5d1d-d44cb0892d65@gmail.com> [-- Attachment #1: Type: text/plain, Size: 3228 bytes --] On Tue, May 11, 2021 at 07:00:34PM +0300, Dmitry Osipenko wrote: > 11.05.2021 18:31, Krzysztof Kozlowski пишет: > ... > ~~~~~~~~~~~~~~~~~~~~~^ > >>>>> drivers/memory/tegra/tegra124-emc.c:802:26: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744071562067985 to 2147483665 [-Wconstant-conversion] > >>> emc_ccfifo_writel(emc, EMC_ZQ_CAL_LONG_CMD_DEV0, EMC_ZQ_CAL); > >>> ~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~ > >>> drivers/memory/tegra/tegra124-emc.c:154:36: note: expanded from macro 'EMC_ZQ_CAL_LONG_CMD_DEV0' > >>> (DRAM_DEV_SEL_0 | EMC_ZQ_CAL_LONG | EMC_ZQ_CAL_CMD) > >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ > >>> 13 warnings generated. > >> > >> This doesn't look like a useful warning from clang, it should see that > >> the constant value itself isn't truncated, hence it should be a problem > >> of clang. Do you think it's okay to ignore this nonsense? > > > > I admit I also do not see the real issue here. The DRAM_DEV_SEL_0 fits > > in u32 and there is no other bitwise arithmetic than just OR, so why > > clang assumes it can have 32 most signifcant bits toggled on? > > > > +Cc Nathan and Nick, > > Maybe you could shed some light here on this warning? > > > > Dmitry, > > In general you should not ignore it because: > > 1. This breaks allyesconfig with clang on powerpc (or it is one of the > > stoppers), > > 2. We might want in some future to build it with clang. > > I meant to ignore it from the perspective of the memory drivers, i.e. it > likely should be fixed in clang and not worked around in the code. Thank > you for pinging the right people. I do not think this is a bug in clang, gcc warns the same (just not here in this case): https://godbolt.org/z/e9GWobMnd DRAM_DEV_SEL_0 and DRAM_DEV_SEL_1 are implicitly signed integers because there is no suffix on the literal 1. DRAM_DEV_SEL_0 is 2 << 30, which can be turned into 1 << 31. That is equal to INT_MAX + 1, which then overflows and becomes INT_MIN (undefined behavior). INT_MIN is then promoted to unsigned long because EMC_ZQ_CAL_LONG and EMC_ZQ_CAL_CMD are unsigned long due to the BIT macro, resulting in the gigantic number that clang reports above. I assume that this driver only runs on hardware where unsigned int is the same size as unsigned long, meaning this problem is merely theoretical? Regardless, defining DRAM_DEV_SEL_{0,1} with the BIT macro fixes the warning for me and should make everything work as expected. diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c index 5699d909abc2..a21ca8e0841a 100644 --- a/drivers/memory/tegra/tegra124-emc.c +++ b/drivers/memory/tegra/tegra124-emc.c @@ -272,8 +272,8 @@ #define EMC_PUTERM_ADJ 0x574 #define DRAM_DEV_SEL_ALL 0 -#define DRAM_DEV_SEL_0 (2 << 30) -#define DRAM_DEV_SEL_1 (1 << 30) +#define DRAM_DEV_SEL_0 BIT(31) +#define DRAM_DEV_SEL_1 BIT(30) #define EMC_CFG_POWER_FEATURES_MASK \ (EMC_CFG_DYN_SREF | EMC_CFG_DRAM_ACPD | EMC_CFG_DRAM_CLKSTOP_SR | \
next prev parent reply other threads:[~2021-05-11 17:35 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-10 21:37 [PATCH v1 0/2] Enable compile-testing for Tegra memory drivers Dmitry Osipenko 2021-05-10 21:37 ` [PATCH v1 1/2] soc/tegra: fuse: Add missing stubs Dmitry Osipenko 2021-05-10 21:37 ` [PATCH v1 2/2] memory: tegra: Enable compile testing for all drivers Dmitry Osipenko 2021-05-11 13:58 ` kernel test robot 2021-05-11 13:58 ` kernel test robot 2021-05-11 15:17 ` Dmitry Osipenko 2021-05-11 15:17 ` Dmitry Osipenko 2021-05-11 15:31 ` Krzysztof Kozlowski 2021-05-11 15:31 ` Krzysztof Kozlowski 2021-05-11 16:00 ` Dmitry Osipenko 2021-05-11 16:00 ` Dmitry Osipenko 2021-05-11 17:35 ` Nathan Chancellor [this message] 2021-05-11 17:35 ` Nathan Chancellor 2021-05-11 17:57 ` Krzysztof Kozlowski 2021-05-11 17:57 ` Krzysztof Kozlowski 2021-05-11 18:36 ` Dmitry Osipenko 2021-05-11 18:36 ` Dmitry Osipenko 2021-05-11 15:43 ` kernel test robot 2021-05-11 15:43 ` kernel test robot 2021-05-11 16:22 ` Dmitry Osipenko 2021-05-11 16:22 ` Dmitry Osipenko 2021-05-11 16:41 ` Krzysztof Kozlowski 2021-05-11 18:30 ` Dmitry Osipenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YJrARxhVD7QM/GPv@archlinux-ax161 \ --to=nathan@kernel.org \ --cc=clang-built-linux@googlegroups.com \ --cc=digetx@gmail.com \ --cc=jonathanh@nvidia.com \ --cc=kbuild-all@lists.01.org \ --cc=krzysztof.kozlowski@canonical.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=lkp@intel.com \ --cc=ndesaulniers@google.com \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.