[PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[Mohammed Shafi Shajakhan]

From: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>

This reverts commit 0ce3bcfc84900a64347b0fe1140229bd81314008.

Event though with the above commit we obtain the configured DTIM period
from the AP rather than always hardcoding it to '1', this seems to cause
problems under the following scenarios:
* Preventing association with broken AP's
* Adds latency in roaming
So its better to always use the safe value of '1' for dtim period

Cc: Jouni Malinen <Jouni.Malinen@Atheros.com>
Signed-off-by: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
---
 drivers/net/wireless/ath/ath9k/htc_drv_init.c |    3 +--
 drivers/net/wireless/ath/ath9k/init.c         |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index 0f6be35..e0a9c83 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -714,8 +714,7 @@ static void ath9k_set_hw_capab(struct ath9k_htc_priv *priv,
 		IEEE80211_HW_HAS_RATE_CONTROL |
 		IEEE80211_HW_RX_INCLUDES_FCS |
 		IEEE80211_HW_SUPPORTS_PS |
-		IEEE80211_HW_PS_NULLFUNC_STACK |
-		IEEE80211_HW_NEED_DTIM_PERIOD;
+		IEEE80211_HW_PS_NULLFUNC_STACK;
 
 	hw->wiphy->interface_modes =
 		BIT(NL80211_IFTYPE_STATION) |
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index b0e5e71..498c3a4 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -647,8 +647,7 @@ void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
 		IEEE80211_HW_SUPPORTS_PS |
 		IEEE80211_HW_PS_NULLFUNC_STACK |
 		IEEE80211_HW_SPECTRUM_MGMT |
-		IEEE80211_HW_REPORTS_TX_ACK_STATUS |
-		IEEE80211_HW_NEED_DTIM_PERIOD;
+		IEEE80211_HW_REPORTS_TX_ACK_STATUS;
 
 	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_HT)
 		 hw->flags |= IEEE80211_HW_AMPDU_AGGREGATION;
-- 
1.7.0.4

Re: [PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[Kalle Valo]

Mohammed Shafi Shajakhan <mshajakhan@atheros.com> writes:

> From: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
>
> This reverts commit 0ce3bcfc84900a64347b0fe1140229bd81314008.
>
> Event though with the above commit we obtain the configured DTIM period
> from the AP rather than always hardcoding it to '1', this seems to cause
> problems under the following scenarios:
> * Preventing association with broken AP's

How can different dtim value cause problems with association? Power
save should not be even enabled during association. Or do you mean
there are problems after association, for example during eap
negotation?

> * Adds latency in roaming

We should try to solve this problem differently than hardcoding dtim.
There are other ways to trigger roaming than just following beacons.
And if there's no data, there's no need to roam either. 

I have been hoping to work on improving roaming, but never found the
time to do it :/

> So its better to always use the safe value of '1' for dtim period

By hardcoding dtim you increase the power consumption in the case when
there is no data traffic, so it's not definitely better in all cases.

I would prefer to fix the real issues instead of hardcoding dtim. And
if it's really essential to hardcode it now, please add a big fat
warning indicating that this is a temporary solution and should be
fixed properly without hardcoding anything.

-- 
Kalle Valo

Re: [PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[Jouni Malinen]

On Sat, Jan 01, 2011 at 11:50:50AM +0200, Kalle Valo wrote:
> Mohammed Shafi Shajakhan <mshajakhan@atheros.com> writes:
> > * Preventing association with broken AP's
> 
> How can different dtim value cause problems with association? Power
> save should not be even enabled during association. Or do you mean
> there are problems after association, for example during eap
> negotation?

It is not the DTIM value, it is the part of having to receive a Beacon
frame before even trying to associate. This is very much a corner case,
but well, that behavior did change with the commit.

> > * Adds latency in roaming
> 
> We should try to solve this problem differently than hardcoding dtim.
> There are other ways to trigger roaming than just following beacons.
> And if there's no data, there's no need to roam either. 

This is about the part of waiting for Beacon frame before trying to
associate, not about hardcoding DTIM for any other purpose.

> > So its better to always use the safe value of '1' for dtim period
> 
> By hardcoding dtim you increase the power consumption in the case when
> there is no data traffic, so it's not definitely better in all cases.
> 
> I would prefer to fix the real issues instead of hardcoding dtim. And
> if it's really essential to hardcode it now, please add a big fat
> warning indicating that this is a temporary solution and should be
> fixed properly without hardcoding anything.

The real issue (as far as I can tell) was that ath9k was changed to
require mac80211 to get it DTIM period before association. I'm not aware
of any need for that with ath9k. It should have no problems updating PS
parameters after association and as such, should not require mac80211 to
figure DTIM period out before being able to associate. This change is
not about hardcoding DTIM period; it is about removing unneeded change
that added extra latency without any real gain.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[Mohammed Shafi]

On Sunday 02 January 2011 02:23 PM, Jouni Malinen wrote:
> On Sat, Jan 01, 2011 at 11:50:50AM +0200, Kalle Valo wrote:
>    
>> Mohammed Shafi Shajakhan<mshajakhan@atheros.com>  writes:
>>      
>>> * Preventing association with broken AP's
>>>        
>> How can different dtim value cause problems with association? Power
>> save should not be even enabled during association. Or do you mean
>> there are problems after association, for example during eap
>> negotation?
>>      
> It is not the DTIM value, it is the part of having to receive a Beacon
> frame before even trying to associate. This is very much a corner case,
> but well, that behavior did change with the commit.
>
>    
>>> * Adds latency in roaming
>>>        
>> We should try to solve this problem differently than hardcoding dtim.
>> There are other ways to trigger roaming than just following beacons.
>> And if there's no data, there's no need to roam either.
>>      
> This is about the part of waiting for Beacon frame before trying to
> associate, not about hardcoding DTIM for any other purpose.
>
>    
>>> So its better to always use the safe value of '1' for dtim period
>>>        
>> By hardcoding dtim you increase the power consumption in the case when
>> there is no data traffic, so it's not definitely better in all cases.
>>   di
>> I would prefer to fix the real issues instead of hardcoding dtim. And
>> if it's really essential to hardcode it now, please add a big fat
>> warning indicating that this is a temporary solution and should be
>> fixed properly without hardcoding anything.
>>      
  I agree .In the ath9k  we wake up for every beacons, and even 
obtaining the DTIM period will not change it, unless we change the wake 
up time to be based on DTIM period. I thought of changing it, but was 
afraid that it might introduce some other critical problems.
> The real issue (as far as I can tell) was that ath9k was changed to
> require mac80211 to get it DTIM period before association. I'm not aware
> of any need for that with ath9k. It should have no problems updating PS
> parameters after association and as such, should not require mac80211 to
> figure DTIM period out before being able to associate. This change is
> not about hardcoding DTIM period; it is about removing unneeded change
> that added extra latency without any real gain.
>
>    
As the power save does not seems to be configured based on this I 
thought there wont be no issues in getting the actual DTIM period, but 
never had a clue this might have introduced these problems.

Re: [PATCH] Revert "ath9k: Parse DTIM period from mac80211"

[Kalle Valo]

Jouni Malinen <j@w1.fi> writes:

> On Sat, Jan 01, 2011 at 11:50:50AM +0200, Kalle Valo wrote:
>> Mohammed Shafi Shajakhan <mshajakhan@atheros.com> writes:
>> > * Preventing association with broken AP's
>> 
>> How can different dtim value cause problems with association? Power
>> save should not be even enabled during association. Or do you mean
>> there are problems after association, for example during eap
>> negotation?
>
> It is not the DTIM value, it is the part of having to receive a Beacon
> frame before even trying to associate. This is very much a corner case,
> but well, that behavior did change with the commit.

[...]

> The real issue (as far as I can tell) was that ath9k was changed to
> require mac80211 to get it DTIM period before association. I'm not aware
> of any need for that with ath9k. It should have no problems updating PS
> parameters after association and as such, should not require mac80211 to
> figure DTIM period out before being able to associate. This change is
> not about hardcoding DTIM period; it is about removing unneeded change
> that added extra latency without any real gain.

Ah, sorry. I misunderstood the case here then.

I have a feeling ath9k consumes quite a lot of power already, even
when I use dtim 10, and I was worried if ath9k power consumption even
increases more :)

-- 
Kalle Valo