[PATCH] d80211: fix sparse warnings

[PATCH] d80211: fix sparse warnings

[Johannes Berg]

This fixes some sparse warnings in d80211.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
A few warnings remain:

net/d80211/ieee80211.c:820:37: warning: potentially expensive pointer subtraction

net/d80211/ieee80211_ioctl.c:779:4: warning: incorrect type in argument 6 (different signedness)
net/d80211/ieee80211_ioctl.c:779:4:    expected int *err
net/d80211/ieee80211_ioctl.c:779:4:    got unsigned int *<noident>

net/d80211/sta_info.c:232:3: warning: context imbalance in 'sta_info_free' - unexpected unlock

The last of these is bogus, the code is perfectly fine. The other two I
don't understand.

---
 net/d80211/ieee80211.c       |    6 +++---
 net/d80211/ieee80211_sta.c   |    6 +++---
 net/d80211/ieee80211_sysfs.c |    1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

--- wireless-dev.orig/net/d80211/ieee80211.c	2007-02-26 23:51:34.284692803 +0100
+++ wireless-dev/net/d80211/ieee80211.c	2007-02-26 23:53:26.114692803 +0100
@@ -2640,7 +2640,7 @@ ieee80211_get_rate(struct ieee80211_loca
 	return NULL;
 }
 
-void
+static void
 ieee80211_fill_frame_info(struct ieee80211_local *local,
 			  struct ieee80211_frame_info *fi,
 			  struct ieee80211_rx_status *status)
@@ -2747,7 +2747,7 @@ ieee80211_rx_mgmt(struct ieee80211_local
 	netif_rx(skb);
 }
 
-void
+static void
 ieee80211_rx_monitor(struct net_device *dev, struct sk_buff *skb,
 		     struct ieee80211_rx_status *status)
 {
@@ -3635,7 +3635,7 @@ static void ieee80211_rx_michael_mic_rep
 
 	if ((rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV) &&
 	    rx->sdata->type == IEEE80211_IF_TYPE_AP) {
-		int keyidx = ieee80211_wep_get_keyidx(rx->skb);
+		keyidx = ieee80211_wep_get_keyidx(rx->skb);
 		/* AP with Pairwise keys support should never receive Michael
 		 * MIC errors for non-zero keyidx because these are reserved
 		 * for group keys and only the AP is sending real multicast
--- wireless-dev.orig/net/d80211/ieee80211_sta.c	2007-02-26 23:55:39.234692803 +0100
+++ wireless-dev/net/d80211/ieee80211_sta.c	2007-02-26 23:57:09.244692803 +0100
@@ -2247,11 +2247,11 @@ static int ieee80211_sta_join_ibss(struc
 
 		rates = 0;
 		for (i = 0; i < bss->supp_rates_len; i++) {
-			int rate = (bss->supp_rates[i] & 0x7f) * 5;
+			int bitrate = (bss->supp_rates[i] & 0x7f) * 5;
 			if (local->hw.conf.phymode == MODE_ATHEROS_TURBO)
-				rate *= 2;
+				bitrate *= 2;
 			for (j = 0; j < local->num_curr_rates; j++)
-				if (local->curr_rates[j].rate == rate)
+				if (local->curr_rates[j].rate == bitrate)
 					rates |= BIT(j);
 		}
 		ifsta->supp_rates_bits = rates;
--- wireless-dev.orig/net/d80211/ieee80211_sysfs.c	2007-02-26 23:56:36.904692803 +0100
+++ wireless-dev/net/d80211/ieee80211_sysfs.c	2007-02-26 23:56:43.854692803 +0100
@@ -16,6 +16,7 @@
 #include <net/cfg80211.h>
 #include "ieee80211_i.h"
 #include "ieee80211_rate.h"
+#include "ieee80211_sysfs.h"
 
 static inline struct ieee80211_local *to_ieee80211_local(struct device *dev)
 {

Re: [PATCH] d80211: fix sparse warnings

[Pavel Roskin]

On Mon, 2007-02-26 at 23:59 +0100, Johannes Berg wrote:
> This fixes some sparse warnings in d80211.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> ---
> A few warnings remain:
> 
> net/d80211/ieee80211.c:820:37: warning: potentially expensive pointer subtraction

That's subtraction of pointers to types with sizes that are not power of
two.  This could lead to expensive division operations.  In may cases,
this warning indicates that using array indices instead of pointers
would be more effective (and probably safer).

> net/d80211/ieee80211_ioctl.c:779:4: warning: incorrect type in argument 6 (different signedness)
> net/d80211/ieee80211_ioctl.c:779:4:    expected int *err
> net/d80211/ieee80211_ioctl.c:779:4:    got unsigned int *<noident>

ieee80211_set_encryption() expects a pointer to (signed) integer, be the
code is giving it a pointer to u32.  Please decide what type "err"
should have, and change the function accordingly.

I think "u32" would be better, since it's a part of the hostapd
compatibility code and we don't want to touch the userspace now.  This
code is going to be obsoleted with the rest of hostapd interface.  For
now, it's better to do exactly what the userspace expects.

> net/d80211/sta_info.c:232:3: warning: context imbalance in 'sta_info_free' - unexpected unlock
> 
> The last of these is bogus, the code is perfectly fine. The other two I
> don't understand.

Great catch!  I have reduced this to a simple test case where sparse
behaves unreasonably, and I'm going to send it to sparse developers as a
bug report.

In case you are wondering, the test case is:

void my_lock(void) __attribute__ ((context(lock, 0, 1)));
void my_unlock(void) __attribute__ ((context(lock, 1, 0)));
void foo(void);
static void sta_info_free(int locked)
{
	if (locked)
		my_lock();
	foo();
	if (locked)
		my_unlock();
}

-- 
Regards,
Pavel Roskin

Re: [PATCH] d80211: fix sparse warnings

[Pavel Roskin]

On Mon, 2007-02-26 at 20:18 -0500, Pavel Roskin wrote:

> Great catch!  I have reduced this to a simple test case where sparse
> behaves unreasonably, and I'm going to send it to sparse developers as a
> bug report.

Linus has replied that it's not a bug:

http://marc.theaimsgroup.com/?l=linux-sparse&m=117254997313979&w=2

He suggests in the end how to avoid that warning.

-- 
Regards,
Pavel Roskin

Re: [PATCH] d80211: fix sparse warnings

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

On Mon, 2007-02-26 at 20:18 -0500, Pavel Roskin wrote:

> That's subtraction of pointers to types with sizes that are not power of
> two.  This could lead to expensive division operations.  In may cases,
> this warning indicates that using array indices instead of pointers
> would be more effective (and probably safer).

Yeah, figured that out after I sent it but then went to bed instead. The
fix in that case is pretty easy, just replace the looping through with
an index-based loop and then you already have rateidx.

> > net/d80211/ieee80211_ioctl.c:779:4: warning: incorrect type in argument 6 (different signedness)
> > net/d80211/ieee80211_ioctl.c:779:4:    expected int *err
> > net/d80211/ieee80211_ioctl.c:779:4:    got unsigned int *<noident>
> 
> ieee80211_set_encryption() expects a pointer to (signed) integer, be the
> code is giving it a pointer to u32.  Please decide what type "err"
> should have, and change the function accordingly.
> 
> I think "u32" would be better, since it's a part of the hostapd
> compatibility code and we don't want to touch the userspace now.  This
> code is going to be obsoleted with the rest of hostapd interface.  For
> now, it's better to do exactly what the userspace expects.

I guess. The argument appears to also not be used in any case other than
this one.

> In case you are wondering, the test case is:

Yeah, I thought it'd be due to the locked flag. But Linus is perfectly
right, we should have a _locked version instead of using a flag.

But on the topic of sparse/locks: Does anybody know how to make sparse
recognise that I provide my own locking wrapper? If you check out
bcm43xx-d80211, it'll complain about the phy lock function since it just
locks, and then complain about phy unlock since it only unlocks. Would
be nice if it could be told that these are opaque, correct, and lock a
certain lock that it should check for in the callers.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] d80211: fix sparse warnings

[Michael Buesch]

On Tuesday 27 February 2007 02:18, Pavel Roskin wrote:
> On Mon, 2007-02-26 at 23:59 +0100, Johannes Berg wrote:
> > This fixes some sparse warnings in d80211.
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > 
> > ---
> > A few warnings remain:
> > 
> > net/d80211/ieee80211.c:820:37: warning: potentially expensive pointer subtraction
> 
> That's subtraction of pointers to types with sizes that are not power of
> two.  This could lead to expensive division operations.  In may cases,
> this warning indicates that using array indices instead of pointers
> would be more effective (and probably safer).

Uh, well.
I think it's this line, no?

	control->rts_rateidx = (int)(rate - tx->local->curr_rates);

The problem there is, we have a pointer into an array and we
need to get the index number into that array.
Any better way to get an index by a pointer and a basepointer?

-- 
Greetings Michael.

Re: [PATCH] d80211: fix sparse warnings

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Tuesday 27 February 2007 10:40, Michael Buesch wrote:
> Uh, well.
> I think it's this line, no?
>
> 	control->rts_rateidx = (int)(rate - tx->local->curr_rates);
>
> The problem there is, we have a pointer into an array and we
> need to get the index number into that array.
> Any better way to get an index by a pointer and a basepointer?
I have a patch that reworks the code totally so this is no longer necessary.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]