ar9170: cleanup of bss_info_changed and beacon config

PATCH Superseded Joerg Albert 3 participants 5 messages Aug 5, 2009 – Aug 7, 2009

Share & tools

Joerg Albert

author

2009-08-05 00:13 UTC #0 lore raw

PATCH 1/2 ar9170: cleanup of bss_info_changed and beacon config

Enable/Disable the beacon in ar910_set_beacon_timers() independently of ar->vif, but controlled by BSS_CHANGED_BEACON_ENABLED and bss_conf->enable_beacon from mac80211.

---
 drivers/net/wireless/ath/ar9170/ar9170.h |    1 +
 drivers/net/wireless/ath/ar9170/mac.c    |    4 +++-
 drivers/net/wireless/ath/ar9170/main.c   |   15 ++++++++-------
 3 files changed, 12 insertions(+), 8 deletions(-)

drivers/net/wireless/ath/ar9170/ar9170.h +1 -0

index e6c3ee3..0db304a 100644
@@ -177,6 +177,7 @@ struct ar9170 {
 	/* beaconing */
 	struct sk_buff *beacon;
 	struct work_struct beacon_work;
+	bool enable_beacon;
 
 	/* cryptographic engine */
 	u64 usedkeys;

drivers/net/wireless/ath/ar9170/mac.c +3 -1

index d9f1f46..9f2801c 100644
@@ -388,7 +388,9 @@ int ar9170_set_beacon_timers(struct ar9170 *ar)
 	u32 v = 0;
 	u32 pretbtt = 0;
 
-	if (ar->vif) {
+	if (ar->enable_beacon) {
+		if (WARN_ON(!ar->vif))
+			return -ENODEV;
 		v |= ar->vif->bss_conf.beacon_int;
 
 		switch (ar->vif->type) {

drivers/net/wireless/ath/ar9170/main.c +8 -7

index 099ed3c..d51b6ad 100644
@@ -2177,11 +2177,18 @@ static void ar9170_op_bss_info_changed(struct ieee80211_hw *hw,
 			goto out;
 	}
 
-	if (changed & (BSS_CHANGED_BEACON | BSS_CHANGED_BEACON_ENABLED)) {
+	if (changed & BSS_CHANGED_BEACON_ENABLED) {
+		ar->enable_beacon = bss_conf->enable_beacon;
+	}
+
+	if (changed & BSS_CHANGED_BEACON) {
 		err = ar9170_update_beacon(ar);
 		if (err)
 			goto out;
+	}
 
+	if (changed & (BSS_CHANGED_BEACON_ENABLED | BSS_CHANGED_BEACON |
+		       BSS_CHANGED_BEACON_INT)) {
 		err = ar9170_set_beacon_timers(ar);
 		if (err)
 			goto out;

@@ -2194,12 +2201,6 @@ static void ar9170_op_bss_info_changed(struct ieee80211_hw *hw,
 #endif /* CONFIG_AR9170_LEDS */
 	}
 
-	if (changed & BSS_CHANGED_BEACON_INT) {
-		err = ar9170_set_beacon_timers(ar);
-		if (err)
-			goto out;
-	}
-
 	if (changed & BSS_CHANGED_HT) {
 		/* TODO */
 		err = 0;

Signature · 1 line

1.6.0.4

Christian Lamparter

2009-08-05 02:04 UTC #1 lore raw

Quoted 3 lines · On Wednesday 05 August 2009 02:13:12 Joerg Albert wrote:

Enable/Disable the beacon in ar910_set_beacon_timers() independently of ar->vif, but controlled by BSS_CHANGED_BEACON_ENABLED and bss_conf->enable_beacon from mac80211.

no signed/cc here?

Quoted 11 lines

---
 drivers/net/wireless/ath/ar9170/ar9170.h |    1 +
 drivers/net/wireless/ath/ar9170/mac.c    |    4 +++-
 drivers/net/wireless/ath/ar9170/main.c   |   15 ++++++++-------
 3 files changed, 12 insertions(+), 8 deletions(-)

drivers/net/wireless/ath/ar9170/mac.c +0 -0

index d9f1f46..9f2801c 100644

drivers/net/wireless/ath/ar9170/mac.c int ar9170_set_beacon_timers(struct ar9170 *ar) · line 396

Quoted 10 lines

drivers/net/wireless/ath/ar9170/mac.c +3 -1

@@ -388,7 +388,9 @@ int ar9170_set_beacon_timers(struct ar9170 *ar)
 	u32 v = 0;
 	u32 pretbtt = 0;
 
-	if (ar->vif) {
+	if (ar->enable_beacon) {
+		if (WARN_ON(!ar->vif))
+			return -ENODEV;
 		v |= ar->vif->bss_conf.beacon_int;
 
 		switch (ar->vif->type) {

err, guess that's why. (not fatal, but WARN)

the beacon timer isn't exclusively used to notify the driver when its time for a new beacon... The STA mode uses the same _timer_ in reverse to wait for the next beacon form the assoc. AP.

that said: It does not look like the firmware implements anything in this direction... But this is a clearly MAC register and there could be something in the silicon which does something useful with this information.

so, to be on the safe side: why not preserve the old behavior for the STA mode as well and simply tell the hardware about dtim & beacon interval?

The only remaining question is where to disabled the timer for STA. (which is in some way relevant to: [PATCH 2/2] because previously,
these timers were always disabled by remove_interface.)

I think the best place is in ar9170_op_bss_info_changed:

       if (changed & BSS_CHANGED_ASSOC) {

just when bss_conf->assoc gets "0".

Regards,
Chr

Joerg Albert

author

2009-08-05 19:05 UTC #2 lore raw

Quoted 1 line · Christian Lamparter

no signed/cc here?

oops, forgot it. Seems like both patches miss some context lines at the end, too.

Quoted 9 lines

Quoted 1 line
...
the beacon timer isn't exclusively used to notify the driver when its time for a new beacon... The STA mode uses the same _timer_ in reverse to wait for the next beacon form the assoc. AP.
that said: It does not look like the firmware implements anything in this direction... But this is a clearly MAC register and there could be something in the silicon which does something useful with this information.

I wasn't aware of this usage within STA mode.

Quoted 2 lines

so, to be on the safe side: why not preserve the old behavior for the STA mode as well and simply tell the hardware about dtim & beacon interval?

ar9170_set_beacon_timers() uses the beacon interval only if for AP mode, dtim is left in STA mode.

Quoted 8 lines

The only remaining question is where to disabled the timer for STA. (which is in some way relevant to: [PATCH 2/2] because previously,
these timers were always disabled by remove_interface.)
I think the best place is in ar9170_op_bss_info_changed:
       if (changed & BSS_CHANGED_ASSOC) {
just when bss_conf->assoc gets "0".

Yes. IMHO also enable on bss_conf->assoc == 1.

I'll look into another version of both patches after running some tests in STA mode.

Regards,
Joerg.

Christian Lamparter

2009-08-05 20:25 UTC #3 lore raw

Quoted 9 lines · On Wednesday 05 August 2009 21:05:09 Joerg Albert wrote:

Quoted 6 lines · Christian Lamparter
the beacon timer isn't exclusively used to notify the driver when its time for a new beacon... The STA mode uses the same _timer_ in reverse to wait for the next beacon form the assoc. AP.
so, to be on the safe side: why not preserve the old behavior for the STA mode as well and simply tell the hardware about dtim & beacon interval?
ar9170_set_beacon_timers() uses the beacon interval only if for AP mode,

Although pretbtt is closely related to the beacon interval.
It has a slightly different purpose: It fires _before_ the beacon is sent. So the host is able to do some _last msec_ changes to the beacon.

Quoted 1 line

dtim is left in STA mode.

both dtim_period and beacon_int are ORed

|
| if (ar->vif) {
|	v |= ar->vif->bss_conf.beacon_int;
|	switch() [...]
|	v |= ar->vif->bss_conf.dtim_period << 16;
| }
|

into AR9170_MAC_REG_BCN_PERIOD.

It would be a BUG if this setting is omitted in STA mode, since dtim beacons are essential.

Quoted 10 lines

Quoted 8 lines
The only remaining question is where to disabled the timer for STA. (which is in some way relevant to: [PATCH 2/2] because previously,
these timers were always disabled by remove_interface.)
I think the best place is in ar9170_op_bss_info_changed:
       if (changed & BSS_CHANGED_ASSOC) {
just when bss_conf->assoc gets "0".
Yes. IMHO also enable on bss_conf->assoc == 1.

yes, but not necessary for bss_conf->assoc == 1, ieee80211_set_associated sets BSS_CHANGED_BEACON_INT.

ar9170_set_beacon_timers is executed twice unless you throw more code at it than just the assoc check & call. But anyway, that's fine by me.

Thanks,
Chr

John W. Linville

2009-08-07 14:46 UTC #4 lore raw

In reply to Joerg Albert

Quoted 5 lines · Joerg Albert

Quoted 1 line · Christian Lamparter
no signed/cc here?
oops, forgot it. Seems like both patches miss some context lines at the end, too.

So for the record, I'm dropping both patches and waiting for a resend with whatever corrections you need to make base on Christian's feedback.

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.