[PATCH 20/38] net: brcm80211: fix range check

[PATCH 20/38] net: brcm80211: fix range check

[Andrzej Hajda]

Unsigned minus constant is still unsigned so checking its sign makes no
sense.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/net/wireless/brcm80211/brcmsmac/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 9728be0..218cbc8 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -4585,7 +4585,7 @@ static int brcms_b_attach(struct brcms_c_info *wlc, struct bcma_device *core,
 		wlc_hw->machwcap_backup = wlc_hw->machwcap;
 
 		/* init tx fifo size */
-		WARN_ON((wlc_hw->corerev - XMTFIFOTBL_STARTREV) < 0 ||
+		WARN_ON(wlc_hw->corerev < XMTFIFOTBL_STARTREV ||
 			(wlc_hw->corerev - XMTFIFOTBL_STARTREV) >
 				ARRAY_SIZE(xmtfifo_sz));
 		wlc_hw->xmtfifo_sz =
-- 
1.9.1

Re: [PATCH 20/38] net: brcm80211: fix range check

[David Miller]

From: Andrzej Hajda <a.hajda@samsung.com>
Date: Mon, 21 Sep 2015 15:33:52 +0200

> Unsigned minus constant is still unsigned so checking its sign makes no
> sense.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Applied.

[PATCH 21/38] mwifiex: fix comparison expression

[Andrzej Hajda]

To avoid underflows signed variables should be used in expression.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/net/wireless/mwifiex/11n_rxreorder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/11n_rxreorder.c b/drivers/net/wireless/mwifiex/11n_rxreorder.c
index 2906cd5..b3970a8 100644
--- a/drivers/net/wireless/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/mwifiex/11n_rxreorder.c
@@ -615,10 +615,10 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private *priv,
 	    ((end_win > start_win) && ((seq_num > end_win) ||
 				       (seq_num < start_win)))) {
 		end_win = seq_num;
-		if (((seq_num - win_size) + 1) >= 0)
+		if (((end_win - win_size) + 1) >= 0)
 			start_win = (end_win - win_size) + 1;
 		else
-			start_win = (MAX_TID_VALUE - (win_size - seq_num)) + 1;
+			start_win = (MAX_TID_VALUE - (win_size - end_win)) + 1;
 		mwifiex_11n_dispatch_pkt_until_start_win(priv, tbl, start_win);
 	}
 
-- 
1.9.1

RE: [PATCH 21/38] mwifiex: fix comparison expression

[Amitkumar Karwar]

Hi Andrzej,

> From: Andrzej Hajda [mailto:a.hajda@samsung.com]
> Sent: Monday, September 21, 2015 7:04 PM
> To: linux-kernel@vger.kernel.org
> Cc: Andrzej Hajda; Bartlomiej Zolnierkiewicz; Marek Szyprowski;
> Amitkumar Karwar; Nishant Sarmukadam; Kalle Valo; linux-
> wireless@vger.kernel.org; netdev@vger.kernel.org
> Subject: [PATCH 21/38] mwifiex: fix comparison expression
> 
> To avoid underflows signed variables should be used in expression.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/net/wireless/mwifiex/11n_rxreorder.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/11n_rxreorder.c
> b/drivers/net/wireless/mwifiex/11n_rxreorder.c
> index 2906cd5..b3970a8 100644
> --- a/drivers/net/wireless/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/mwifiex/11n_rxreorder.c
> @@ -615,10 +615,10 @@ int mwifiex_11n_rx_reorder_pkt(struct
> mwifiex_private *priv,
>  	    ((end_win > start_win) && ((seq_num > end_win) ||
>  				       (seq_num < start_win)))) {
>  		end_win = seq_num;
> -		if (((seq_num - win_size) + 1) >= 0)
> +		if (((end_win - win_size) + 1) >= 0)
>  			start_win = (end_win - win_size) + 1;
>  		else
> -			start_win = (MAX_TID_VALUE - (win_size - seq_num)) +
> 1;
> +			start_win = (MAX_TID_VALUE - (win_size - end_win)) +
> 1;
>  		mwifiex_11n_dispatch_pkt_until_start_win(priv, tbl,
> start_win);
>  	}
> 

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

Re: [PATCH 21/38] mwifiex: fix comparison expression

[David Miller]

From: Andrzej Hajda <a.hajda@samsung.com>
Date: Mon, 21 Sep 2015 15:33:53 +0200

> To avoid underflows signed variables should be used in expression.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Applied.

[PATCH 22/38] orinoco: fix checking for default value

[Andrzej Hajda]

Thresholds uses -1 to indicate that default value should be used.
Since thresholds are unsigned sign checking makes no sense.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/net/wireless/orinoco/cfg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/orinoco/cfg.c b/drivers/net/wireless/orinoco/cfg.c
index a9e94b6..0f6ea31 100644
--- a/drivers/net/wireless/orinoco/cfg.c
+++ b/drivers/net/wireless/orinoco/cfg.c
@@ -220,7 +220,7 @@ static int orinoco_set_wiphy_params(struct wiphy *wiphy, u32 changed)
 	if (changed & WIPHY_PARAM_FRAG_THRESHOLD) {
 		/* Set fragmentation */
 		if (priv->has_mwo) {
-			if (wiphy->frag_threshold < 0)
+			if (wiphy->frag_threshold == -1)
 				frag_value = 0;
 			else {
 				printk(KERN_WARNING "%s: Fixed fragmentation "
@@ -230,7 +230,7 @@ static int orinoco_set_wiphy_params(struct wiphy *wiphy, u32 changed)
 				frag_value = 1;
 			}
 		} else {
-			if (wiphy->frag_threshold < 0)
+			if (wiphy->frag_threshold == -1)
 				frag_value = 2346;
 			else if ((wiphy->frag_threshold < 257) ||
 				 (wiphy->frag_threshold > 2347))
@@ -252,7 +252,7 @@ static int orinoco_set_wiphy_params(struct wiphy *wiphy, u32 changed)
 		 * the upper limit.
 		 */
 
-		if (wiphy->rts_threshold < 0)
+		if (wiphy->rts_threshold == -1)
 			rts_value = 2347;
 		else if (wiphy->rts_threshold > 2347)
 			err = -EINVAL;
-- 
1.9.1

Re: [PATCH 22/38] orinoco: fix checking for default value

[David Miller]

From: Andrzej Hajda <a.hajda@samsung.com>
Date: Mon, 21 Sep 2015 15:33:54 +0200

> Thresholds uses -1 to indicate that default value should be used.
> Since thresholds are unsigned sign checking makes no sense.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Applied.

[PATCH 23/38] rndis_wlan: fix checking for default value

[Andrzej Hajda]

Thresholds uses -1 to indicate that default value should be used.
Since thresholds are unsigned sign checking makes no sense.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/net/wireless/rndis_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 71a825c..a13d1f2 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -1236,7 +1236,7 @@ static int set_rts_threshold(struct usbnet *usbdev, u32 rts_threshold)
 
 	netdev_dbg(usbdev->net, "%s(): %i\n", __func__, rts_threshold);
 
-	if (rts_threshold < 0 || rts_threshold > 2347)
+	if (rts_threshold == -1 || rts_threshold > 2347)
 		rts_threshold = 2347;
 
 	tmp = cpu_to_le32(rts_threshold);
-- 
1.9.1

Re: [PATCH 23/38] rndis_wlan: fix checking for default value

[David Miller]

From: Andrzej Hajda <a.hajda@samsung.com>
Date: Mon, 21 Sep 2015 15:33:55 +0200

> Thresholds uses -1 to indicate that default value should be used.
> Since thresholds are unsigned sign checking makes no sense.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Applied.

Re: [PATCH 00/38] Fixes related to incorrect usage of unsigned types

[David Howells]

Andrzej Hajda <a.hajda@samsung.com> wrote:

> Semantic patch finds comparisons of types:
>     unsigned < 0
>     unsigned >= 0
> The former is always false, the latter is always true.
> Such comparisons are useless, so theoretically they could be
> safely removed, but their presence quite often indicates bugs.

Or someone has left them in because they don't matter and there's the
possibility that the type being tested might be or become signed under some
circumstances.  If the comparison is useless, I'd expect the compiler to just
discard it - for such cases your patch is pointless.

If I have, for example:

	unsigned x;

	if (x == 0 || x > 27)
		give_a_range_error();

I will write this as:

	unsigned x;

	if (x <= 0 || x > 27)
		give_a_range_error();

because it that gives a way to handle x being changed to signed at some point
in the future for no cost.  In which case, your changing the <= to an ==
"because the < part of the case is useless" is arguably wrong.

David

Re: [PATCH 00/38] Fixes related to incorrect usage of unsigned types

[Andrzej Hajda]

On 09/21/2015 03:42 PM, David Howells wrote:
> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> Semantic patch finds comparisons of types:
>>     unsigned < 0
>>     unsigned >= 0
>> The former is always false, the latter is always true.
>> Such comparisons are useless, so theoretically they could be
>> safely removed, but their presence quite often indicates bugs.
> 
> Or someone has left them in because they don't matter and there's the
> possibility that the type being tested might be or become signed under some
> circumstances.  If the comparison is useless, I'd expect the compiler to just
> discard it - for such cases your patch is pointless.
> 
> If I have, for example:
> 
> 	unsigned x;
> 
> 	if (x == 0 || x > 27)
> 		give_a_range_error();
> 
> I will write this as:
> 
> 	unsigned x;
> 
> 	if (x <= 0 || x > 27)
> 		give_a_range_error();
> 
> because it that gives a way to handle x being changed to signed at some point
> in the future for no cost.  In which case, your changing the <= to an ==
> "because the < part of the case is useless" is arguably wrong.

This is why I have not checked for such cases - I have skipped checks of type
	unsigned <= 0
exactly for the reasons above.

However I have left two other checks as they seems to me more suspicious - they
are always true or false. But as Dmitry and Andrew pointed out Linus have quite
strong opinion against removing range checks in such cases as he finds it
clearer. I think it applies to patches 29-36. I am not sure about patches 26-28,37.

Regards
Andrzej

> 
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 00/38] Fixes related to incorrect usage of unsigned types

[Jacek Anaszewski]

On 09/22/2015 11:13 AM, Andrzej Hajda wrote:
> On 09/21/2015 03:42 PM, David Howells wrote:
>> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>> Semantic patch finds comparisons of types:
>>>      unsigned < 0
>>>      unsigned >= 0
>>> The former is always false, the latter is always true.
>>> Such comparisons are useless, so theoretically they could be
>>> safely removed, but their presence quite often indicates bugs.
>>
>> Or someone has left them in because they don't matter and there's the
>> possibility that the type being tested might be or become signed under some
>> circumstances.  If the comparison is useless, I'd expect the compiler to just
>> discard it - for such cases your patch is pointless.
>>
>> If I have, for example:
>>
>> 	unsigned x;
>>
>> 	if (x == 0 || x > 27)
>> 		give_a_range_error();
>>
>> I will write this as:
>>
>> 	unsigned x;
>>
>> 	if (x <= 0 || x > 27)
>> 		give_a_range_error();
>>
>> because it that gives a way to handle x being changed to signed at some point
>> in the future for no cost.  In which case, your changing the <= to an ==
>> "because the < part of the case is useless" is arguably wrong.
>
> This is why I have not checked for such cases - I have skipped checks of type
> 	unsigned <= 0
> exactly for the reasons above.
>
> However I have left two other checks as they seems to me more suspicious - they
> are always true or false. But as Dmitry and Andrew pointed out Linus have quite
> strong opinion against removing range checks in such cases as he finds it
> clearer. I think it applies to patches 29-36. I am not sure about patches 26-28,37.

Dropped 30/38 and 31/38 from LED tree then.

-- 
Best Regards,
Jacek Anaszewski