linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
@ 2021-10-14 21:57 Nathan Chancellor
  2021-10-15  9:43 ` Dan Carpenter
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Nathan Chancellor @ 2021-10-14 21:57 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nick Desaulniers, linux-staging, linux-kernel, llvm, Nathan Chancellor

A new warning in clang points out a place in this file where a bitwise
OR is being used with boolean expressions:

In file included from drivers/staging/wlan-ng/prism2usb.c:2:
drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
            ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
            ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
1 warning generated.

The comment explains that short circuiting here is undesirable, as the
calls to test_and_{clear,set}_bit() need to happen for both sides of the
expression.

Clang's suggestion would work to silence the warning but the readability
of the expression would suffer even more. To clean up the warning and
make the block more readable, use a variable for each side of the
bitwise expression.

Link: https://github.com/ClangBuiltLinux/linux/issues/1478
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
index 59aa84d1837d..938e11a1a0b6 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
 
 	spin_lock_irqsave(&hw->ctlxq.lock, flags);
 
-	/*
-	 * We need to check BOTH the RX and the TX throttle controls,
-	 * so we use the bitwise OR instead of the logical OR.
-	 */
 	pr_debug("flags=0x%lx\n", hw->usb_flags);
-	if (!hw->wlandev->hwremoved &&
-	    ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
-	      !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
-	     (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
-	      !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
-	    )) {
-		schedule_work(&hw->usb_work);
+	if (!hw->wlandev->hwremoved) {
+		bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
+				   !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
+		bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
+				   !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
+		/*
+		 * We need to check BOTH the RX and the TX throttle controls,
+		 * so we use the bitwise OR instead of the logical OR.
+		 */
+		if (rx_throttle | tx_throttle)
+			schedule_work(&hw->usb_work);
 	}
 
 	spin_unlock_irqrestore(&hw->ctlxq.lock, flags);

base-commit: 6ac113f741a7674e4268eea3eb13972732d83571
-- 
2.33.1.637.gf443b226ca


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-14 21:57 [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn() Nathan Chancellor
@ 2021-10-15  9:43 ` Dan Carpenter
  2021-10-15 17:13   ` Nathan Chancellor
  2021-10-18 20:23   ` Nick Desaulniers
  2021-10-18 20:24 ` Nick Desaulniers
  2021-10-20  8:14 ` David Laight
  2 siblings, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2021-10-15  9:43 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Greg Kroah-Hartman, Nick Desaulniers, linux-staging, linux-kernel, llvm

On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> A new warning in clang points out a place in this file where a bitwise
> OR is being used with boolean expressions:
> 
> In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
>             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
>             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> 1 warning generated.

Both sides of this bitwise OR are bool, so | and || are equivalent
logically.  Clang should not warn about it.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-15  9:43 ` Dan Carpenter
@ 2021-10-15 17:13   ` Nathan Chancellor
  2021-10-16  7:04     ` Dan Carpenter
  2021-10-18 20:23   ` Nick Desaulniers
  1 sibling, 1 reply; 8+ messages in thread
From: Nathan Chancellor @ 2021-10-15 17:13 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, Nick Desaulniers, linux-staging, linux-kernel, llvm

On Fri, Oct 15, 2021 at 12:43:44PM +0300, Dan Carpenter wrote:
> On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> > A new warning in clang points out a place in this file where a bitwise
> > OR is being used with boolean expressions:
> > 
> > In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> >             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> >             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> > 1 warning generated.
> 
> Both sides of this bitwise OR are bool, so | and || are equivalent
> logically.  Clang should not warn about it.

I do not disagree. The original motivation for the warning was code like

if (a() & b())

where a '&&' was intended to short circuit the call to b() if a() was
false but then it expanded to encompass bitwise OR as well. The clang
developers felt that warning on bitwise OR was worthwhile because most
of the time, '||' was intended. Feel free to comment on the Phabricator
thread if you feel strongly, there are not too many instances of this
warning and I think the '&' vs '&&' aspect of the warning is useful.

https://reviews.llvm.org/D108003

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-15 17:13   ` Nathan Chancellor
@ 2021-10-16  7:04     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2021-10-16  7:04 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Greg Kroah-Hartman, Nick Desaulniers, linux-staging, linux-kernel, llvm

On Fri, Oct 15, 2021 at 10:13:05AM -0700, Nathan Chancellor wrote:
> On Fri, Oct 15, 2021 at 12:43:44PM +0300, Dan Carpenter wrote:
> > On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> > > A new warning in clang points out a place in this file where a bitwise
> > > OR is being used with boolean expressions:
> > > 
> > > In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> > >             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> > >             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> > > 1 warning generated.
> > 
> > Both sides of this bitwise OR are bool, so | and || are equivalent
> > logically.  Clang should not warn about it.
> 
> I do not disagree. The original motivation for the warning was code like
> 
> if (a() & b())
> 
> where a '&&' was intended to short circuit the call to b() if a() was
> false but then it expanded to encompass bitwise OR as well. The clang
> developers felt that warning on bitwise OR was worthwhile because most
> of the time, '||' was intended. Feel free to comment on the Phabricator
> thread if you feel strongly, there are not too many instances of this
> warning and I think the '&' vs '&&' aspect of the warning is useful.

Of course, this was a Smatch check early on.  Bool is almost all style
debates and false positives.  I didn't see a lot of short circuiting
bugs in the kernel.

But I'm not going to tell people how to live their lifes if they want to
spend it debating coding style.  (Unless it's GCC's brain dead unsigned
comparison warnings where it tells everyone to make their iterators
unsigned int and leads to a lot bugs and ugly code).

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-15  9:43 ` Dan Carpenter
  2021-10-15 17:13   ` Nathan Chancellor
@ 2021-10-18 20:23   ` Nick Desaulniers
  2021-11-01 12:49     ` Dan Carpenter
  1 sibling, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2021-10-18 20:23 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Nathan Chancellor, Greg Kroah-Hartman, linux-staging, linux-kernel, llvm

On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> > A new warning in clang points out a place in this file where a bitwise
> > OR is being used with boolean expressions:
> >
> > In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> >             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> >             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> > 1 warning generated.
>
> Both sides of this bitwise OR are bool, so | and || are equivalent
> logically.  Clang should not warn about it.

Not when the LHS AND RHS of the the binary operator have side effects,
which is the only condition under which this warning is emitted.  RHS
potentially sets a bit, and potentially would not be executed if `|`
was replaced with `||`.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-14 21:57 [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn() Nathan Chancellor
  2021-10-15  9:43 ` Dan Carpenter
@ 2021-10-18 20:24 ` Nick Desaulniers
  2021-10-20  8:14 ` David Laight
  2 siblings, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2021-10-18 20:24 UTC (permalink / raw)
  To: Nathan Chancellor; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, llvm

On Thu, Oct 14, 2021 at 2:57 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> A new warning in clang points out a place in this file where a bitwise
> OR is being used with boolean expressions:
>
> In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
>             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
>             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> 1 warning generated.
>
> The comment explains that short circuiting here is undesirable, as the
> calls to test_and_{clear,set}_bit() need to happen for both sides of the
> expression.
>
> Clang's suggestion would work to silence the warning but the readability
> of the expression would suffer even more. To clean up the warning and
> make the block more readable, use a variable for each side of the
> bitwise expression.

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

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1478
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> index 59aa84d1837d..938e11a1a0b6 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
>
>         spin_lock_irqsave(&hw->ctlxq.lock, flags);
>
> -       /*
> -        * We need to check BOTH the RX and the TX throttle controls,
> -        * so we use the bitwise OR instead of the logical OR.
> -        */
>         pr_debug("flags=0x%lx\n", hw->usb_flags);
> -       if (!hw->wlandev->hwremoved &&
> -           ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> -             !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
> -            (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> -             !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
> -           )) {
> -               schedule_work(&hw->usb_work);
> +       if (!hw->wlandev->hwremoved) {
> +               bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> +                                  !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
> +               bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> +                                  !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
> +               /*
> +                * We need to check BOTH the RX and the TX throttle controls,
> +                * so we use the bitwise OR instead of the logical OR.
> +                */
> +               if (rx_throttle | tx_throttle)
> +                       schedule_work(&hw->usb_work);
>         }
>
>         spin_unlock_irqrestore(&hw->ctlxq.lock, flags);
>
> base-commit: 6ac113f741a7674e4268eea3eb13972732d83571
> --
> 2.33.1.637.gf443b226ca
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-14 21:57 [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn() Nathan Chancellor
  2021-10-15  9:43 ` Dan Carpenter
  2021-10-18 20:24 ` Nick Desaulniers
@ 2021-10-20  8:14 ` David Laight
  2 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2021-10-20  8:14 UTC (permalink / raw)
  To: 'Nathan Chancellor', Greg Kroah-Hartman
  Cc: Nick Desaulniers, linux-staging, linux-kernel, llvm

From: Nathan Chancellor
> Sent: 14 October 2021 22:57
> 
> A new warning in clang points out a place in this file where a bitwise
> OR is being used with boolean expressions:
> 
> In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-
> Wbitwise-instead-of-logical]
>             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
>             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this
> warning
> 1 warning generated.
> 
> The comment explains that short circuiting here is undesirable, as the
> calls to test_and_{clear,set}_bit() need to happen for both sides of the
> expression.
> 
> Clang's suggestion would work to silence the warning but the readability
> of the expression would suffer even more. To clean up the warning and
> make the block more readable, use a variable for each side of the
> bitwise expression.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/1478
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> index 59aa84d1837d..938e11a1a0b6 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
> 
>  	spin_lock_irqsave(&hw->ctlxq.lock, flags);
> 
> -	/*
> -	 * We need to check BOTH the RX and the TX throttle controls,
> -	 * so we use the bitwise OR instead of the logical OR.
> -	 */
>  	pr_debug("flags=0x%lx\n", hw->usb_flags);
> -	if (!hw->wlandev->hwremoved &&
> -	    ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> -	      !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
> -	     (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> -	      !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
> -	    )) {
> -		schedule_work(&hw->usb_work);
> +	if (!hw->wlandev->hwremoved) {
> +		bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> +				   !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
> +		bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> +				   !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
> +		/*
> +		 * We need to check BOTH the RX and the TX throttle controls,
> +		 * so we use the bitwise OR instead of the logical OR.
> +		 */
> +		if (rx_throttle | tx_throttle)
> +			schedule_work(&hw->usb_work);

Why not the slightly simpler:
		bool throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
				   !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
		throttle |= test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
				   !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
		if (throttle)
			schedule_work(&hw->usb_work);
or with s/throttle/throttle_silly_compiler_warning/ :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
  2021-10-18 20:23   ` Nick Desaulniers
@ 2021-11-01 12:49     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2021-11-01 12:49 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Greg Kroah-Hartman, linux-staging, linux-kernel, llvm

On Mon, Oct 18, 2021 at 01:23:45PM -0700, Nick Desaulniers wrote:
> On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> > > A new warning in clang points out a place in this file where a bitwise
> > > OR is being used with boolean expressions:
> > >
> > > In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> > >             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> > >             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> > > 1 warning generated.
> >
> > Both sides of this bitwise OR are bool, so | and || are equivalent
> > logically.  Clang should not warn about it.
> 
> Not when the LHS AND RHS of the the binary operator have side effects,
> which is the only condition under which this warning is emitted.  RHS
> potentially sets a bit, and potentially would not be executed if `|`
> was replaced with `||`.

Yes.  But as in this case, if you silenced the "bitwise-instead-of-logical"
warning in the "obvious way" by changing the | to || then it will
introduce a bug so it's a risky warning.

Ideally everyone would try to understand the code they are changing but
that's just not true in real life.  What happens is that every single
new person compiles the code and sees the warning.  There is only one
or two people who understand the driver code and a hundred people who
compile the code and look at warnings.  So there is a slightly over 98%
chance that the person looking at the warning doesn't understand the
code and they're going to try to fix it.  But instead they're going to
introduce a bug because they're going to change | to ||.

Of course, Nathan and I are a bit experienced so we're careful but other
people are less careful and we've seen this over and over where risky
warnings just introduce bugs.  I saw this a month ago where Smatch
complained about a missing error code.  It was ancient code so
the original author was not arround.  A junior dev saw the bug and
changed it to return -EINVAL.  That Smatch check is very high quality
and it did look like it was supposed to be an error path so the patch
was back ported to stable.  But it turned out that return path was
supposed to be success so it broke the boot.

I haven't looked in detail.  I silenced these warnings in Smatch because
my impression at the time was that there was a high false positive rate.
That's still my impression, but I haven't looked.  It might also be
different for different code bases.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-11-01 12:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 21:57 [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn() Nathan Chancellor
2021-10-15  9:43 ` Dan Carpenter
2021-10-15 17:13   ` Nathan Chancellor
2021-10-16  7:04     ` Dan Carpenter
2021-10-18 20:23   ` Nick Desaulniers
2021-11-01 12:49     ` Dan Carpenter
2021-10-18 20:24 ` Nick Desaulniers
2021-10-20  8:14 ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).