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

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

[Joerg Albert]

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(-)

diff --git a/drivers/net/wireless/ath/ar9170/ar9170.h b/drivers/net/wireless/ath/ar9170/ar9170.h
index e6c3ee3..0db304a 100644
--- a/drivers/net/wireless/ath/ar9170/ar9170.h
+++ b/drivers/net/wireless/ath/ar9170/ar9170.h
@@ -177,6 +177,7 @@ struct ar9170 {
 	/* beaconing */
 	struct sk_buff *beacon;
 	struct work_struct beacon_work;
+	bool enable_beacon;
 
 	/* cryptographic engine */
 	u64 usedkeys;
diff --git a/drivers/net/wireless/ath/ar9170/mac.c b/drivers/net/wireless/ath/ar9170/mac.c
index d9f1f46..9f2801c 100644
--- a/drivers/net/wireless/ath/ar9170/mac.c
+++ b/drivers/net/wireless/ath/ar9170/mac.c
@@ -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) {
diff --git a/drivers/net/wireless/ath/ar9170/main.c b/drivers/net/wireless/ath/ar9170/main.c
index 099ed3c..d51b6ad 100644
--- a/drivers/net/wireless/ath/ar9170/main.c
+++ b/drivers/net/wireless/ath/ar9170/main.c
@@ -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;
-- 
1.6.0.4

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

[Christian Lamparter]

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?

> ---
>  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(-)
> 
> diff --git a/drivers/net/wireless/ath/ar9170/mac.c b/drivers/net/wireless/ath/ar9170/mac.c
> index d9f1f46..9f2801c 100644
> --- a/drivers/net/wireless/ath/ar9170/mac.c
> +++ b/drivers/net/wireless/ath/ar9170/mac.c
> @@ -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

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

[Joerg Albert]

On 08/05/2009 04:04 AM, Christian Lamparter wrote:

> no signed/cc here?

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

> > ...
> 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.

> 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.

> 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.

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

[Christian Lamparter]

On Wednesday 05 August 2009 21:05:09 Joerg Albert wrote:
> On 08/05/2009 04:04 AM, Christian Lamparter wrote:
> > 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.

> 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.
> > 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

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

[John W. Linville]

On Wed, Aug 05, 2009 at 09:05:09PM +0200, Joerg Albert wrote:
> On 08/05/2009 04:04 AM, Christian Lamparter wrote:
> 
> > 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
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.