This fixes some sparse warnings in d80211.
Signed-off-by: Johannes Berg <[email protected]>
---
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)
{
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 <[email protected]>
> >
> > ---
> > 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.
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
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
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
On Mon, 2007-02-26 at 23:59 +0100, Johannes Berg wrote:
> This fixes some sparse warnings in d80211.
>
> Signed-off-by: Johannes Berg <[email protected]>
>
> ---
> 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