* [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning
@ 2021-10-21 21:45 Nathan Chancellor
2021-10-21 21:55 ` Michał Mirosław
2021-11-17 18:21 ` Nick Desaulniers
0 siblings, 2 replies; 4+ messages in thread
From: Nathan Chancellor @ 2021-10-21 21:45 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter
Cc: Nick Desaulniers, linux-tegra, linux-kernel, llvm, Nathan Chancellor
A new warning in clang points out two instances where boolean
expressions are being used with a bitwise OR instead of logical OR:
drivers/soc/tegra/fuse/speedo-tegra20.c:72:9: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
reg = tegra_fuse_read_spare(i) |
^~~~~~~~~~~~~~~~~~~~~~~~~~
||
drivers/soc/tegra/fuse/speedo-tegra20.c:72:9: note: cast one or both operands to int to silence this warning
drivers/soc/tegra/fuse/speedo-tegra20.c:87:9: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
reg = tegra_fuse_read_spare(i) |
^~~~~~~~~~~~~~~~~~~~~~~~~~
||
drivers/soc/tegra/fuse/speedo-tegra20.c:87:9: note: cast one or both operands to int to silence this warning
2 warnings generated.
The motivation for the warning is that logical operations short circuit
while bitwise operations do not. In this case, it does not seem like
short circuiting is harmful so implement the suggested fix of changing
to a logical operation to fix the warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/1488
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
drivers/soc/tegra/fuse/speedo-tegra20.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/tegra/fuse/speedo-tegra20.c b/drivers/soc/tegra/fuse/speedo-tegra20.c
index 2546bddbab93..4984246605ae 100644
--- a/drivers/soc/tegra/fuse/speedo-tegra20.c
+++ b/drivers/soc/tegra/fuse/speedo-tegra20.c
@@ -69,7 +69,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
val = 0;
for (i = CPU_SPEEDO_MSBIT; i >= CPU_SPEEDO_LSBIT; i--) {
- reg = tegra_fuse_read_spare(i) |
+ reg = tegra_fuse_read_spare(i) ||
tegra_fuse_read_spare(i + CPU_SPEEDO_REDUND_OFFS);
val = (val << 1) | (reg & 0x1);
}
@@ -84,7 +84,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
val = 0;
for (i = SOC_SPEEDO_MSBIT; i >= SOC_SPEEDO_LSBIT; i--) {
- reg = tegra_fuse_read_spare(i) |
+ reg = tegra_fuse_read_spare(i) ||
tegra_fuse_read_spare(i + SOC_SPEEDO_REDUND_OFFS);
val = (val << 1) | (reg & 0x1);
}
base-commit: 519d81956ee277b4419c723adfb154603c2565ba
--
2.33.1.637.gf443b226ca
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning
2021-10-21 21:45 [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning Nathan Chancellor
@ 2021-10-21 21:55 ` Michał Mirosław
2021-10-25 14:34 ` Nathan Chancellor
2021-11-17 18:21 ` Nick Desaulniers
1 sibling, 1 reply; 4+ messages in thread
From: Michał Mirosław @ 2021-10-21 21:55 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Thierry Reding, Jonathan Hunter, Nick Desaulniers, linux-tegra,
linux-kernel, llvm
On Thu, Oct 21, 2021 at 02:45:00PM -0700, Nathan Chancellor wrote:
[...]
> --- a/drivers/soc/tegra/fuse/speedo-tegra20.c
> +++ b/drivers/soc/tegra/fuse/speedo-tegra20.c
> @@ -69,7 +69,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
>
> val = 0;
> for (i = CPU_SPEEDO_MSBIT; i >= CPU_SPEEDO_LSBIT; i--) {
> - reg = tegra_fuse_read_spare(i) |
> + reg = tegra_fuse_read_spare(i) ||
> tegra_fuse_read_spare(i + CPU_SPEEDO_REDUND_OFFS);
> val = (val << 1) | (reg & 0x1);
> }
> @@ -84,7 +84,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
>
> val = 0;
> for (i = SOC_SPEEDO_MSBIT; i >= SOC_SPEEDO_LSBIT; i--) {
> - reg = tegra_fuse_read_spare(i) |
> + reg = tegra_fuse_read_spare(i) ||
> tegra_fuse_read_spare(i + SOC_SPEEDO_REDUND_OFFS);
> val = (val << 1) | (reg & 0x1);
> }
It does seem correct, but nevertheless the code looks suspicious. reg is
already masked with 0x1 as far as I can tell, and there are other places
which depend on this (like speedo-tegra210.c). Guessing from the use of
tegra_fuse_read_spare() I would recommend changing its return type as it
is returing a bit value, not necessarily semantically a boolean.
Best Regards
Michał Mirosław
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning
2021-10-21 21:55 ` Michał Mirosław
@ 2021-10-25 14:34 ` Nathan Chancellor
0 siblings, 0 replies; 4+ messages in thread
From: Nathan Chancellor @ 2021-10-25 14:34 UTC (permalink / raw)
To: Michał Mirosław
Cc: Thierry Reding, Jonathan Hunter, Nick Desaulniers, linux-tegra,
linux-kernel, llvm
Hi Michał,
On Thu, Oct 21, 2021 at 11:55:34PM +0200, Michał Mirosław wrote:
> On Thu, Oct 21, 2021 at 02:45:00PM -0700, Nathan Chancellor wrote:
> [...]
> > --- a/drivers/soc/tegra/fuse/speedo-tegra20.c
> > +++ b/drivers/soc/tegra/fuse/speedo-tegra20.c
> > @@ -69,7 +69,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
> >
> > val = 0;
> > for (i = CPU_SPEEDO_MSBIT; i >= CPU_SPEEDO_LSBIT; i--) {
> > - reg = tegra_fuse_read_spare(i) |
> > + reg = tegra_fuse_read_spare(i) ||
> > tegra_fuse_read_spare(i + CPU_SPEEDO_REDUND_OFFS);
> > val = (val << 1) | (reg & 0x1);
> > }
> > @@ -84,7 +84,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
> >
> > val = 0;
> > for (i = SOC_SPEEDO_MSBIT; i >= SOC_SPEEDO_LSBIT; i--) {
> > - reg = tegra_fuse_read_spare(i) |
> > + reg = tegra_fuse_read_spare(i) ||
> > tegra_fuse_read_spare(i + SOC_SPEEDO_REDUND_OFFS);
> > val = (val << 1) | (reg & 0x1);
> > }
>
> It does seem correct, but nevertheless the code looks suspicious. reg is
> already masked with 0x1 as far as I can tell, and there are other places
> which depend on this (like speedo-tegra210.c). Guessing from the use of
> tegra_fuse_read_spare() I would recommend changing its return type as it
> is returing a bit value, not necessarily semantically a boolean.
Yes, I did notice that, as well as the use of tegra_fuse_read_spare()
with boolean operators in tegra-apbmisc.c. I could change it to int if
that is what the maintainers prefer, which would also solve the warning.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning
2021-10-21 21:45 [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning Nathan Chancellor
2021-10-21 21:55 ` Michał Mirosław
@ 2021-11-17 18:21 ` Nick Desaulniers
1 sibling, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2021-11-17 18:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Thierry Reding, Jonathan Hunter, linux-tegra, linux-kernel, llvm,
Anders Roxell
+ Anders (combining
https://lore.kernel.org/all/20211117142115.694316-1-anders.roxell@linaro.org/).
On Thu, Oct 21, 2021 at 2:45 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> A new warning in clang points out two instances where boolean
> expressions are being used with a bitwise OR instead of logical OR:
>
> drivers/soc/tegra/fuse/speedo-tegra20.c:72:9: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> reg = tegra_fuse_read_spare(i) |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ||
> drivers/soc/tegra/fuse/speedo-tegra20.c:72:9: note: cast one or both operands to int to silence this warning
> drivers/soc/tegra/fuse/speedo-tegra20.c:87:9: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> reg = tegra_fuse_read_spare(i) |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ||
> drivers/soc/tegra/fuse/speedo-tegra20.c:87:9: note: cast one or both operands to int to silence this warning
> 2 warnings generated.
>
> The motivation for the warning is that logical operations short circuit
> while bitwise operations do not. In this case, it does not seem like
> short circuiting is harmful so implement the suggested fix of changing
> to a logical operation to fix the warning.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1488
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Fixes: 25cd5a391478 ("ARM: tegra: Add speedo-based process identification")
> ---
> drivers/soc/tegra/fuse/speedo-tegra20.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/tegra/fuse/speedo-tegra20.c b/drivers/soc/tegra/fuse/speedo-tegra20.c
> index 2546bddbab93..4984246605ae 100644
> --- a/drivers/soc/tegra/fuse/speedo-tegra20.c
> +++ b/drivers/soc/tegra/fuse/speedo-tegra20.c
> @@ -69,7 +69,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
>
> val = 0;
> for (i = CPU_SPEEDO_MSBIT; i >= CPU_SPEEDO_LSBIT; i--) {
> - reg = tegra_fuse_read_spare(i) |
> + reg = tegra_fuse_read_spare(i) ||
> tegra_fuse_read_spare(i + CPU_SPEEDO_REDUND_OFFS);
> val = (val << 1) | (reg & 0x1);
> }
> @@ -84,7 +84,7 @@ void __init tegra20_init_speedo_data(struct tegra_sku_info *sku_info)
>
> val = 0;
> for (i = SOC_SPEEDO_MSBIT; i >= SOC_SPEEDO_LSBIT; i--) {
> - reg = tegra_fuse_read_spare(i) |
> + reg = tegra_fuse_read_spare(i) ||
> tegra_fuse_read_spare(i + SOC_SPEEDO_REDUND_OFFS);
> val = (val << 1) | (reg & 0x1);
> }
>
> base-commit: 519d81956ee277b4419c723adfb154603c2565ba
> --
> 2.33.1.637.gf443b226ca
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-17 18:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 21:45 [PATCH] soc/tegra: fuse: Fix bitwise vs. logical OR warning Nathan Chancellor
2021-10-21 21:55 ` Michał Mirosław
2021-10-25 14:34 ` Nathan Chancellor
2021-11-17 18:21 ` Nick Desaulniers
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.