[PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[Mohammed Shafi Shajakhan]

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

Add few comments for not parsing DTIM period from mac80211

Signed-off-by: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
---
 drivers/net/wireless/ath/ath9k/beacon.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index 87ba44c..fcb36ab 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -721,8 +721,9 @@ void ath_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif)
 		cur_conf->beacon_interval = 100;
 
 	/*
-	 * Some times we dont parse dtim period from mac80211, in that case
-	 * use a default value
+	 * We don't parse dtim period from mac80211 during the driver
+	 * initialization as it breaks association with hidden-ssid
+	 * AP and it causes latency in roaming
 	 */
 	if (cur_conf->dtim_period == 0)
 		cur_conf->dtim_period = 1;
-- 
1.7.0.4

Re: [PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[Björn Smedman]

On Mon, Feb 7, 2011 at 4:38 PM, Mohammed Shafi Shajakhan
<mshajakhan@atheros.com> wrote:
> From: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
>
> Add few comments for not parsing DTIM period from mac80211
>
> Signed-off-by: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
> ---
>  drivers/net/wireless/ath/ath9k/beacon.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> index 87ba44c..fcb36ab 100644
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -721,8 +721,9 @@ void ath_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif)
>                cur_conf->beacon_interval = 100;
>
>        /*
> -        * Some times we dont parse dtim period from mac80211, in that case
> -        * use a default value
> +        * We don't parse dtim period from mac80211 during the driver
> +        * initialization as it breaks association with hidden-ssid
> +        * AP and it causes latency in roaming
>         */
>        if (cur_conf->dtim_period == 0)
>                cur_conf->dtim_period = 1;

I realize I'm not the perfect representative for the intended audience
but I don't understand that comment at all. The previous one made
sense to me, but the new one seems to refer to some logic somewhere
else. In ath_beacon_config() I expect AP/IBSS logic but the comment
seems to be about some special case for a STA vif associating with a
hidden access point, no? I don't understand that special case either
but that is more easily attributable to ignorance. :)

Best regards,

Björn

Re: [PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[Mohammed Shafi]

On Wednesday 09 February 2011 02:52 AM, Björn Smedman wrote:
> On Mon, Feb 7, 2011 at 4:38 PM, Mohammed Shafi Shajakhan
> <mshajakhan@atheros.com>  wrote:
>    
>> From: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>
>> Add few comments for not parsing DTIM period from mac80211
>>
>> Signed-off-by: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>> ---
>>   drivers/net/wireless/ath/ath9k/beacon.c |    5 +++--
>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
>> index 87ba44c..fcb36ab 100644
>> --- a/drivers/net/wireless/ath/ath9k/beacon.c
>> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
>> @@ -721,8 +721,9 @@ void ath_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif)
>>                 cur_conf->beacon_interval = 100;
>>
>>         /*
>> -        * Some times we dont parse dtim period from mac80211, in that case
>> -        * use a default value
>> +        * We don't parse dtim period from mac80211 during the driver
>> +        * initialization as it breaks association with hidden-ssid
>> +        * AP and it causes latency in roaming
>>          */
>>         if (cur_conf->dtim_period == 0)
>>                 cur_conf->dtim_period = 1;
>>      
> I realize I'm not the perfect representative for the intended audience
> but I don't understand that comment at all. The previous one made
> sense to me, but the new one seems to refer to some logic somewhere
> else. In ath_beacon_config() I expect AP/IBSS logic but the comment
> seems to be about some special case for a STA vif associating with a
> hidden access point, no? I don't understand that special case either
> but that is more easily attributable to ignorance. :)
>    
I also have don't have much idea. I saw that by parsing the DTIM 
period(in init.c ,IEEE80211_HW_NEED_DTIM_PERIOD) from the mac80211 it 
broke association with hidden-SSID AP .I shall dig deep into this in future.

thanks,
shafi

> Best regards,
>
> Björn
> .
>
>    

Re: [PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[Jouni Malinen]

On Tue, Feb 08, 2011 at 10:22:39PM +0100, Björn Smedman wrote:
> On Mon, Feb 7, 2011 at 4:38 PM, Mohammed Shafi Shajakhan
> <mshajakhan@atheros.com> wrote:
> > -        * Some times we dont parse dtim period from mac80211, in that case
> > -        * use a default value
> > +        * We don't parse dtim period from mac80211 during the driver
> > +        * initialization as it breaks association with hidden-ssid
> > +        * AP and it causes latency in roaming

> I realize I'm not the perfect representative for the intended audience
> but I don't understand that comment at all. The previous one made
> sense to me, but the new one seems to refer to some logic somewhere
> else. In ath_beacon_config() I expect AP/IBSS logic but the comment
> seems to be about some special case for a STA vif associating with a
> hidden access point, no? I don't understand that special case either
> but that is more easily attributable to ignorance. :)

While I agree that the proposed comment is quite confusing, this
function is actually used even in station mode to set beacon timers for
when to wake up for receiving beacon frames, etc. However, I don't see
why this comment should really be here. In addition, this should really
be fixed by updating DTIM period after the first Beacon frame is
received. Forcing a wait for that Beacon frame is the one that causes
the extra latency; I don't remember why exactly this would break
hidden SSID case unless there is another bug somewhere.

ath9k does not need to know the DTIM period before association and the
first call to this function may need to handle the 0 -> 1 safety check.
This should then be followed by another call (likely new code in
mac80211 needed for this) to update the timers after the DTIM period has
been learned post-association.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 1/1] ath9k: Update comments for not parsing DTIM period

[Mohammed Shafi]

On Wednesday 09 February 2011 08:08 PM, Jouni Malinen wrote:
> On Tue, Feb 08, 2011 at 10:22:39PM +0100, Björn Smedman wrote:
>    
>> On Mon, Feb 7, 2011 at 4:38 PM, Mohammed Shafi Shajakhan
>> <mshajakhan@atheros.com>  wrote:
>>      
>>> -        * Some times we dont parse dtim period from mac80211, in that case
>>> -        * use a default value
>>> +        * We don't parse dtim period from mac80211 during the driver
>>> +        * initialization as it breaks association with hidden-ssid
>>> +        * AP and it causes latency in roaming
>>>        
>    
>> I realize I'm not the perfect representative for the intended audience
>> but I don't understand that comment at all. The previous one made
>> sense to me, but the new one seems to refer to some logic somewhere
>> else. In ath_beacon_config() I expect AP/IBSS logic but the comment
>> seems to be about some special case for a STA vif associating with a
>> hidden access point, no? I don't understand that special case either
>> but that is more easily attributable to ignorance. :)
>>      
> While I agree that the proposed comment is quite confusing, this
> function is actually used even in station mode to set beacon timers for
> when to wake up for receiving beacon frames, etc. However, I don't see
> why this comment should really be here. In addition, this should really
> be fixed by updating DTIM period after the first Beacon frame is
> received. Forcing a wait for that Beacon frame is the one that causes
> the extra latency; I don't remember why exactly this would break
> hidden SSID case unless there is another bug somewhere.
>    
Jouni I had validate few times with our AP and it does broke assoc with 
hidden SSID AP.

thanks,
shafi

> ath9k does not need to know the DTIM period before association and the
> first call to this function may need to handle the 0 ->  1 safety check.
> This should then be followed by another call (likely new code in
> mac80211 needed for this) to update the timers after the DTIM period has
> been learned post-association.
>
>