linux-kernel - Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=gqDM=i=RRSgPEtkqYEDBod8C=FTf1SZ1V4U4KMLgGJA@mail.gmail.com>
Date:   Mon, 18 Oct 2021 13:24:25 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()

On Thu, Oct 14, 2021 at 2:57 PM Nathan Chancellor <nathan@...nel.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@...gle.com>

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1478
> Signed-off-by: Nathan Chancellor <nathan@...nel.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