diff mbox series

nfp: bpf: Fix bitwise vs. logical OR warning

Message ID 20211018193101.2340261-1-nathan@kernel.org (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series nfp: bpf: Fix bitwise vs. logical OR warning | expand

Checks

Context Check Description
netdev/cover_letter success Single patches do not need cover letters
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 8 maintainers not CCed: andrii@kernel.org daniel@iogearbox.net songliubraving@fb.com yhs@fb.com john.fastabend@gmail.com kafai@fb.com ast@kernel.org kpsingh@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success No Fixes tag
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Nathan Chancellor Oct. 18, 2021, 7:31 p.m. UTC 
A new warning in clang points out two places in this driver where
boolean expressions are being used with a bitwise OR instead of a
logical one:

drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
        reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                             ||
drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: note: cast one or both operands to int to silence this warning
drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
        reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                             ||
drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: note: cast one or both operands to int to silence this warning
2 errors 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/1479
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/net/ethernet/netronome/nfp/nfp_asm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 041c61488236a5a84789083e3d9f0a51139b6edf

Comments

Nick Desaulniers Oct. 18, 2021, 7:39 p.m. UTC | #1 
On Mon, Oct 18, 2021 at 12:31 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> A new warning in clang points out two places in this driver where
> boolean expressions are being used with a bitwise OR instead of a
> logical one:
>
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                                              ||
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: note: cast one or both operands to int to silence this warning
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                                              ||
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: note: cast one or both operands to int to silence this warning
> 2 errors 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.

I agree. Thanks for the patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1479
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_asm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_asm.c b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> index 2643ea5948f4..154399c5453f 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> @@ -196,7 +196,7 @@ int swreg_to_unrestricted(swreg dst, swreg lreg, swreg rreg,
>         }
>
>         reg->dst_lmextn = swreg_lmextn(dst);
> -       reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> +       reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
>
>         return 0;
>  }
> @@ -277,7 +277,7 @@ int swreg_to_restricted(swreg dst, swreg lreg, swreg rreg,
>         }
>
>         reg->dst_lmextn = swreg_lmextn(dst);
> -       reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> +       reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
>
>         return 0;
>  }
>
> base-commit: 041c61488236a5a84789083e3d9f0a51139b6edf
> --
> 2.33.1.637.gf443b226ca
>
>
Nick Desaulniers Oct. 18, 2021, 7:40 p.m. UTC | #2 
On Mon, Oct 18, 2021 at 12:39 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Oct 18, 2021 at 12:31 PM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > A new warning in clang points out two places in this driver where
> > boolean expressions are being used with a bitwise OR instead of a
> > logical one:
> >
> > drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> >                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >                                              ||
> > drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: note: cast one or both operands to int to silence this warning
> > drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> >                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >                                              ||
> > drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: note: cast one or both operands to int to silence this warning
> > 2 errors 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.
>
> I agree. Thanks for the patch.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Perhaps:

Fixes: 995e101ffa71 ("nfp: bpf: encode extended LM pointer operands")

>
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1479
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >  drivers/net/ethernet/netronome/nfp/nfp_asm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_asm.c b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> > index 2643ea5948f4..154399c5453f 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
> > @@ -196,7 +196,7 @@ int swreg_to_unrestricted(swreg dst, swreg lreg, swreg rreg,
> >         }
> >
> >         reg->dst_lmextn = swreg_lmextn(dst);
> > -       reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> > +       reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
> >
> >         return 0;
> >  }
> > @@ -277,7 +277,7 @@ int swreg_to_restricted(swreg dst, swreg lreg, swreg rreg,
> >         }
> >
> >         reg->dst_lmextn = swreg_lmextn(dst);
> > -       reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
> > +       reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
> >
> >         return 0;
> >  }
> >
> > base-commit: 041c61488236a5a84789083e3d9f0a51139b6edf
> > --
> > 2.33.1.637.gf443b226ca
> >
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
Jakub Kicinski Oct. 18, 2021, 9:54 p.m. UTC | #3 
On Mon, 18 Oct 2021 12:31:01 -0700 Nathan Chancellor wrote:
> A new warning in clang points out two places in this driver where
> boolean expressions are being used with a bitwise OR instead of a
> logical one:
> 
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                                              ||
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:199:20: note: cast one or both operands to int to silence this warning
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>         reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                                              ||
> drivers/net/ethernet/netronome/nfp/nfp_asm.c:280:20: note: cast one or both operands to int to silence this warning
> 2 errors 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.

Warning seems generally useful, although in this case it is a little
out of place (swreg_lmextn() is a field extractor after all).

Applied to net, but without the Fixes tag, thanks!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_asm.c b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
index 2643ea5948f4..154399c5453f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_asm.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
@@ -196,7 +196,7 @@  int swreg_to_unrestricted(swreg dst, swreg lreg, swreg rreg,
 	}
 
 	reg->dst_lmextn = swreg_lmextn(dst);
-	reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
+	reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
 
 	return 0;
 }
@@ -277,7 +277,7 @@  int swreg_to_restricted(swreg dst, swreg lreg, swreg rreg,
 	}
 
 	reg->dst_lmextn = swreg_lmextn(dst);
-	reg->src_lmextn = swreg_lmextn(lreg) | swreg_lmextn(rreg);
+	reg->src_lmextn = swreg_lmextn(lreg) || swreg_lmextn(rreg);
 
 	return 0;
 }