From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Chancellor <nathan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev
Subject: Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
Date: Mon, 18 Oct 2021 13:24:25 -0700 [thread overview]
Message-ID: <CAKwvOd=gqDM=i=RRSgPEtkqYEDBod8C=FTf1SZ1V4U4KMLgGJA@mail.gmail.com> (raw)
In-Reply-To: <20211014215703.3705371-1-nathan@kernel.org>
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
next prev parent reply other threads:[~2021-10-18 20:24 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-10-20 8:14 ` David Laight
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='CAKwvOd=gqDM=i=RRSgPEtkqYEDBod8C=FTf1SZ1V4U4KMLgGJA@mail.gmail.com' \
--to=ndesaulniers@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).