[PATCH v2] p54: add missing parentheses in p54_flush()

[PATCH v2] p54: add missing parentheses in p54_flush()

[Rustam Subkhankulov]

The assignment of the value to the variable total in the loop
condition must be enclosed in additional parentheses, since otherwise,
in accordance with the precedence of the operators, the conjunction
will be performed first, and only then the assignment.

Due to this error, a warning later in the function after the loop may
not occur in the situation when it should.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
Fixes: 0d4171e2153b ("p54: implement flush callback")
---
 drivers/net/wireless/intersil/p54/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
index a3ca6620dc0c..8fa3ec71603e 100644
--- a/drivers/net/wireless/intersil/p54/main.c
+++ b/drivers/net/wireless/intersil/p54/main.c
@@ -682,7 +682,7 @@ static void p54_flush(struct ieee80211_hw *dev, struct ieee80211_vif *vif,
 	 * queues have already been stopped and no new frames can sneak
 	 * up from behind.
 	 */
-	while ((total = p54_flush_count(priv) && i--)) {
+	while ((total = p54_flush_count(priv)) && i--) {
 		/* waste time */
 		msleep(20);
 	}
-- 
2.25.1

Re: [PATCH v2] p54: add missing parentheses in p54_flush()

[Christian Lamparter]

On 14/07/2022 15:48, Rustam Subkhankulov wrote:
> The assignment of the value to the variable total in the loop
> condition must be enclosed in additional parentheses, since otherwise,
> in accordance with the precedence of the operators, the conjunction
> will be performed first, and only then the assignment.
> 
> Due to this error, a warning later in the function after the loop may
> not occur in the situation when it should.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Fixes: 0d4171e2153b ("p54: implement flush callback")

For reference: This is the "warning" the commit message talks about:
| WARN(total, "tx flush timeout, unresponsive firmware");
| } // this is right at the end of the p54_flush() function


from what I can tell, the difference between:

|	while ((total = p54_flush_count(priv) && i--)) {

and

|	while ((total = p54_flush_count(priv)) && i--) {

boils down to what that "total" ends up being after the
while() has run through.

In the original code it can either be 0 (for everything is ok)
or 1 (still pending - this is bad / firmware is on the fritz).

In the patched version "total" will be 0 or the value of
p54_flush_count(priv).

I think both the current and the patched version behave
the same way and produce the same output.

However I think (since the variable is named "total")
the returned value of p54_flush_count() is indeed more
precise here.

Acked-by: Christian Lamparter <chunkeey@gmail.com>

> ---
>   drivers/net/wireless/intersil/p54/main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index a3ca6620dc0c..8fa3ec71603e 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -682,7 +682,7 @@ static void p54_flush(struct ieee80211_hw *dev, struct ieee80211_vif *vif,
>   	 * queues have already been stopped and no new frames can sneak
>   	 * up from behind.
>   	 */
> -	while ((total = p54_flush_count(priv) && i--)) {
> +	while ((total = p54_flush_count(priv)) && i--) {
>   		/* waste time */
>   		msleep(20);
>   	}

Re: [v2] wifi: p54: add missing parentheses in p54_flush()

[Kalle Valo]

Rustam Subkhankulov <subkhankulov@ispras.ru> wrote:

> The assignment of the value to the variable total in the loop
> condition must be enclosed in additional parentheses, since otherwise,
> in accordance with the precedence of the operators, the conjunction
> will be performed first, and only then the assignment.
> 
> Due to this error, a warning later in the function after the loop may
> not occur in the situation when it should.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Fixes: 0d4171e2153b ("p54: implement flush callback")
> Acked-by: Christian Lamparter <chunkeey@gmail.com>

Patch applied to wireless-next.git, thanks.

bcfd9d7f6840 wifi: p54: add missing parentheses in p54_flush()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220714134831.106004-1-subkhankulov@ispras.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches