Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Sat, 6 Jan 2007 20:09:36 +0100, Ivo Van Doorn wrote: [...] @@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct net_device *dev, The patch is line-wrapped here. Applied, but please try to fix your mail client settings. Thanks for the patch, Jiri -- Jiri Benc SUSE Labs - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
Ivo van Doorn wrote:
+#define __bss_tim_set(__bss, __aid) __set_bit((__aid), (__bss)-tim)
+
__set/clear_bit demands unsigned long, tim is u8. That causes quite some
warnings here.
...
static inline void bss_tim_clear(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
spin_lock(local-sta_lock);
- bss-tim[(aid)/8] = !(1((aid) % 8));
+ __bss_tim_clear(bss, aid);
spin_unlock(local-sta_lock);
Probably forgotten: we need _bh here as well.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Tue, 2007-01-02 at 16:22 +, Christoph Hellwig wrote: This really screams to be converted to __set_bit. Whoops, I should really have jumped into this thread earlier but somehow missed it. This cannot be converted to __set_bit because the IEEE specs mandate this format. We can insert a comment that the format must stay like this :) You have to realise that the TIM is sent as-is with the 0th bit indicating multicast traffic (IIRC) and each of the other bits indicating traffic for that AID. johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote: This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused. NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames. johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Sat, 2007-01-06 at 17:52 +0100, Johannes Berg wrote: On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote: This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused. NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames. Which, in fact, is the only point we keep track of the TIM, note that we never actually test if bits there are set, the stack has better ways of keeping track of the pending traffic. johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
Johannes Berg wrote: On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote: This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused. NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames. Bit ordering, I see. Then go for my original patch + comments why this is open-coded in __bss_tim_set/clear + removed unused arguments from those functions, OK? Jan signature.asc Description: OpenPGP digital signature
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Sat, 2007-01-06 at 18:00 +0100, Jan Kiszka wrote: Johannes Berg wrote: On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote: This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused. NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames. Bit ordering, I see. Then go for my original patch + comments why this is open-coded in __bss_tim_set/clear + removed unused arguments from those functions, OK? Sounds good to me, care to send a new patch? johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
Bit ordering, I see. Then go for my original patch + comments why this
is open-coded in __bss_tim_set/clear + removed unused arguments from
those functions, OK?
Sounds good to me, care to send a new patch?
This patch returns to the original format for setting and clearing the tim bit,
as the IEEE specs mandate. (Comment has been added to prevent future
attempts to use the __set_bit and __clear_bit)
And the local argument has been removed.
Signed-off-by Jan Kiszka [EMAIL PROTECTED]
Signed-off-by Ivo van Doorn [EMAIL PROTECTED]
---
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index ef303da..64d881c 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -558,20 +558,38 @@ struct sta_attribute {
ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
};
+static inline void __bss_tim_set(struct ieee80211_if_ap *bss, int aid)
+{
+ /*
+* This format has ben mandated by the IEEE specifications,
+* so this line may not be changed to use the __set_bit() format.
+*/
+ bss-tim[(aid)/8] |= 1((aid) % 8);
+}
+
static inline void bss_tim_set(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
- spin_lock(local-sta_lock);
- bss-tim[(aid)/8] |= 1((aid) % 8);
- spin_unlock(local-sta_lock);
+ spin_lock_bh(local-sta_lock);
+ __bss_tim_set(bss, aid);
+ spin_unlock_bh(local-sta_lock);
+}
+
+static inline void __bss_tim_clear(struct ieee80211_if_ap *bss, int aid)
+{
+ /*
+* This format has ben mandated by the IEEE specifications,
+* so this line may not be changed to use the __clear_bit() format.
+*/
+ bss-tim[(aid)/8] = !(1((aid) % 8));
}
static inline void bss_tim_clear(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
- spin_lock(local-sta_lock);
- bss-tim[(aid)/8] = !(1((aid) % 8));
- spin_unlock(local-sta_lock);
+ spin_lock_bh(local-sta_lock);
+ __bss_tim_clear(bss, aid);
+ spin_unlock_bh(local-sta_lock);
}
/* ieee80211.c */
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index c74b431..1363a01 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -285,7 +285,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,
if (sta-dev != dev) {
/* Binding STA to a new interface, so remove all references to
* the old BSS. */
+ spin_lock_bh(local-sta_lock);
sta_info_remove_aid_ptr(sta);
+ spin_unlock_bh(local-sta_lock);
}
/* TODO
@@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct
net_device *dev,
sta = sta_info_get(local, param-sta_addr);
if (sta) {
sta_info_put(sta);
- sta_info_free(sta, 1);
+ sta_info_free(sta, 0);
}
return sta ? 0 : -ENOENT;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..f27bd0e 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)
sdata-local-ops-set_tim(local_to_hw(sdata-local),
sta-aid, 0);
if (sdata-bss)
- bss_tim_clear(sdata-local, sdata-bss, sta-aid);
+ __bss_tim_clear(sdata-bss, sta-aid);
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On Tuesday 02 January 2007 17:22, Christoph Hellwig wrote:
On Tue, Jan 02, 2007 at 04:30:41PM +0100, Ivo Van Doorn wrote:
+static inline void __bss_tim_set(struct ieee80211_local *local,
+struct ieee80211_if_ap *bss, int aid)
+{
+ bss-tim[(aid)/8] |= 1((aid) % 8);
+}
This really screams to be converted to __set_bit. Also the local
argument is entirely unused. I'd probaby not even add a helper for
this but just opencode it as:
__set_bit(bss-time, aid);
+static inline void __bss_tim_clear(struct ieee80211_local *local,
+ struct ieee80211_if_ap *bss, int aid)
+{
+ bss-tim[(aid)/8] = !(1((aid) % 8));
Similarly this should just be __clear_bit
This patch uses the __set_bit and __clear_but as suggested by Christoph.
It also removes the local argument since it was unused.
Signed-off-by: Jan Kiszka [EMAIL PROTECTED]
Signed-off-by Ivo van Doorn [EMAIL PROTECTED]
---
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index 6ba6a61..c5c04d2 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -560,19 +560,23 @@ struct sta_attribute {
ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
};
+#define __bss_tim_set(__bss, __aid)__set_bit((__aid), (__bss)-tim)
+
static inline void bss_tim_set(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
- spin_lock(local-sta_lock);
- bss-tim[(aid)/8] |= 1((aid) % 8);
- spin_unlock(local-sta_lock);
+ spin_lock_bh(local-sta_lock);
+ __bss_tim_set(bss, aid);
+ spin_unlock_bh(local-sta_lock);
}
+#define __bss_tim_clear(__bss, __aid) __clear_bit((__aid), (__bss)-tim)
+
static inline void bss_tim_clear(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
spin_lock(local-sta_lock);
- bss-tim[(aid)/8] = !(1((aid) % 8));
+ __bss_tim_clear(bss, aid);
spin_unlock(local-sta_lock);
}
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index 81a6e82..5a21b2d 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -286,7 +286,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,
if (sta-dev != dev) {
/* Binding STA to a new interface, so remove all references to
* the old BSS. */
+ spin_lock_bh(local-sta_lock);
sta_info_remove_aid_ptr(sta);
+ spin_unlock_bh(local-sta_lock);
}
/* TODO
@@ -360,7 +362,7 @@ static int ieee80211_ioctl_remove_sta(struct net_device
*dev,
sta = sta_info_get(local, param-sta_addr);
if (sta) {
sta_info_put(sta);
- sta_info_free(sta, 1);
+ sta_info_free(sta, 0);
}
return sta ? 0 : -ENOENT;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..f27bd0e 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)
sdata-local-ops-set_tim(local_to_hw(sdata-local),
sta-aid, 0);
if (sdata-bss)
- bss_tim_clear(sdata-local, sdata-bss, sta-aid);
+ __bss_tim_clear(sdata-bss, sta-aid);
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] d80211: Fix inconsistent sta_lock usage
On 1/1/07, Jan Kiszka [EMAIL PROTECTED] wrote:
Hacking a bit on rt2x00 to make it work in master and ad-hoc mode, lockdep
popped up on some hostapd ioctls, pointing out remaining inconsistencies
related to sta_lock:
1. sta_lock holders must always be protected against softirq
2. bss_tim_set/clear must not be called with sta_lock held, rather an
unprotected variant
3. ieee80211_ioctl_remove_sta is not already holding the lock when calling
sta_info_free
As I was not sure if sta_info_remove_aid_ptr needs lock protection or
not, I played safe and moved it always under the lock. Please correct me
if this is overkill.
Signed-off-by: Jan Kiszka [EMAIL PROTECTED]
[Sorry, patch is against rt2x00 CVS. I'm lacking time and bandwidth to pull
the d80211 git repos and rebase.]
To make it easier for everybody, here is the same patch
only this time applied to the dscape git tree. ;)
Signed-off-by: Jan Kiszka [EMAIL PROTECTED]
Signed-off-by: Ivo van Doorn [EMAIL PROTECTED]
---
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index ef303da..b132ae0 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -558,20 +558,32 @@ struct sta_attribute {
ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
};
+static inline void __bss_tim_set(struct ieee80211_local *local,
+struct ieee80211_if_ap *bss, int aid)
+{
+ bss-tim[(aid)/8] |= 1((aid) % 8);
+}
+
static inline void bss_tim_set(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
- spin_lock(local-sta_lock);
- bss-tim[(aid)/8] |= 1((aid) % 8);
- spin_unlock(local-sta_lock);
+ spin_lock_bh(local-sta_lock);
+ __bss_tim_set(local, bss, aid);
+ spin_unlock_bh(local-sta_lock);
+}
+
+static inline void __bss_tim_clear(struct ieee80211_local *local,
+ struct ieee80211_if_ap *bss, int aid)
+{
+ bss-tim[(aid)/8] = !(1((aid) % 8));
}
static inline void bss_tim_clear(struct ieee80211_local *local,
struct ieee80211_if_ap *bss, int aid)
{
- spin_lock(local-sta_lock);
- bss-tim[(aid)/8] = !(1((aid) % 8));
- spin_unlock(local-sta_lock);
+ spin_lock_bh(local-sta_lock);
+ __bss_tim_clear(local, bss, aid);
+ spin_unlock_bh(local-sta_lock);
}
/* ieee80211.c */
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index c74b431..1363a01 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -285,7 +285,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,
if (sta-dev != dev) {
/* Binding STA to a new interface, so remove all references to
* the old BSS. */
+ spin_lock_bh(local-sta_lock);
sta_info_remove_aid_ptr(sta);
+ spin_unlock_bh(local-sta_lock);
}
/* TODO
@@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct
net_device *dev,
sta = sta_info_get(local, param-sta_addr);
if (sta) {
sta_info_put(sta);
- sta_info_free(sta, 1);
+ sta_info_free(sta, 0);
}
return sta ? 0 : -ENOENT;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..e120a4f 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)
sdata-local-ops-set_tim(local_to_hw(sdata-local),
sta-aid, 0);
if (sdata-bss)
- bss_tim_clear(sdata-local, sdata-bss, sta-aid);
+ __bss_tim_clear(sdata-local, sdata-bss, sta-aid);
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
