[PATCH] Revert "ath9k: Calculate sleep duration"

[PATCH] Revert "ath9k: Calculate sleep duration"

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

This reverts commit 09ebb810927a110e4c354beb20308830d108a54b.

ath9k_hw_set_sta_beacon_timers() configures AR_TIM_PERIOD with
the beacon interval. Before this commit, the sleepduration was
never greater than the beacon interval. But now, the behavior
has changed. For example, with an AP that uses a beacon interval of 100:

ath: phy9: next beacon 61128704
ath: phy9: beacon period 204800
ath: phy9: DTIM period 204800

If the sleepduration is calculated based on the listen time, then
the bmiss threshold should also be changed since the HW would
be in sleep state for a longer time, but that is not done currently.

To avoid configuring a higher beacon interval based on the sleepduration,
revert to the original behavior. Power consumption is not a
problem since PS is disabled in ath9k anyway.

Cc: stable@vger.kernel.org
Cc: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/common-beacon.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/common-beacon.c b/drivers/net/wireless/ath/ath9k/common-beacon.c
index 733be51..775d1d2 100644
--- a/drivers/net/wireless/ath/ath9k/common-beacon.c
+++ b/drivers/net/wireless/ath/ath9k/common-beacon.c
@@ -57,7 +57,7 @@ int ath9k_cmn_beacon_config_sta(struct ath_hw *ah,
 				 struct ath9k_beacon_state *bs)
 {
 	struct ath_common *common = ath9k_hw_common(ah);
-	int dtim_intval, sleepduration;
+	int dtim_intval;
 	u64 tsf;
 
 	/* No need to configure beacon if we are not associated */
@@ -75,7 +75,6 @@ int ath9k_cmn_beacon_config_sta(struct ath_hw *ah,
 	 * last beacon we received (which may be none).
 	 */
 	dtim_intval = conf->intval * conf->dtim_period;
-	sleepduration = ah->hw->conf.listen_interval * conf->intval;
 
 	/*
 	 * Pull nexttbtt forward to reflect the current
@@ -113,7 +112,7 @@ int ath9k_cmn_beacon_config_sta(struct ath_hw *ah,
 	 */
 
 	bs->bs_sleepduration = TU_TO_USEC(roundup(IEEE80211_MS_TO_TU(100),
-						  sleepduration));
+						 conf->intval));
 	if (bs->bs_sleepduration > bs->bs_dtimperiod)
 		bs->bs_sleepduration = bs->bs_dtimperiod;
 
-- 
2.1.0

Re: [PATCH] Revert "ath9k: Calculate sleep duration"

[Rajkumar Manoharan]

On Thu, Sep 11, 2014 at 07:05:48PM +0530, Sujith Manoharan wrote:
> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
> 
> This reverts commit 09ebb810927a110e4c354beb20308830d108a54b.
> 
> ath9k_hw_set_sta_beacon_timers() configures AR_TIM_PERIOD with
> the beacon interval. Before this commit, the sleepduration was
> never greater than the beacon interval. But now, the behavior
> has changed. For example, with an AP that uses a beacon interval of 100:
> 
> ath: phy9: next beacon 61128704
> ath: phy9: beacon period 204800
> ath: phy9: DTIM period 204800
> 
> If the sleepduration is calculated based on the listen time, then
> the bmiss threshold should also be changed since the HW would
> be in sleep state for a longer time, but that is not done currently.
> 
> To avoid configuring a higher beacon interval based on the sleepduration,
> revert to the original behavior. Power consumption is not a
> problem since PS is disabled in ath9k anyway.
>
Some of the IOE customer using ath9k with power save enabled. IIRC the
expectation is that when the station is idle, it should wakeup on DTIM
beacon not for every beacons. Are you seeing frequest bmiss events on
higher beacon interval?

-Rajkumar

Re: [PATCH] Revert "ath9k: Calculate sleep duration"

[Sujith Manoharan]

Rajkumar Manoharan wrote:
> Some of the IOE customer using ath9k with power save enabled. IIRC the
> expectation is that when the station is idle, it should wakeup on DTIM
> beacon not for every beacons. Are you seeing frequest bmiss events on
> higher beacon interval?

I don't think ath9k has ever been tested with longer sleep durations
based on dtim period. We have always programmed the hardware to wake up
for every beacon interval, using the TIM_TIMER interrupt. And since the
TIM bit needs to be checked for every beacon, the sleep duration has
to be the beacon interval.

Also, the bmiss threshold needs to be adjusted based on the sleep
duration. We are hardcoding it currently in the driver using
ATH_DEFAULT_BMISS_LIMIT. This will break, too.

Using dtim for PS needs more testing before it can be used,
so we can have the original behavior for now.

Sujith

Re: [PATCH] Revert "ath9k: Calculate sleep duration"

[Rajkumar Manoharan]

On Thu, Sep 11, 2014 at 07:56:31PM +0530, Sujith Manoharan wrote:
> Rajkumar Manoharan wrote:
> > Some of the IOE customer using ath9k with power save enabled. IIRC the
> > expectation is that when the station is idle, it should wakeup on DTIM
> > beacon not for every beacons. Are you seeing frequest bmiss events on
> > higher beacon interval?
> 
> I don't think ath9k has ever been tested with longer sleep durations
> based on dtim period. We have always programmed the hardware to wake up
> for every beacon interval, using the TIM_TIMER interrupt. And since the
> TIM bit needs to be checked for every beacon, the sleep duration has
> to be the beacon interval.
> 
> Also, the bmiss threshold needs to be adjusted based on the sleep
> duration. We are hardcoding it currently in the driver using
> ATH_DEFAULT_BMISS_LIMIT. This will break, too.
> 
> Using dtim for PS needs more testing before it can be used,
> so we can have the original behavior for now.
> 
Sounds clear.

Acked-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>

Re: [PATCH] Revert "ath9k: Calculate sleep duration"

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Am 12.09.2014 um 08:09 schrieb Rajkumar Manoharan:
> On Thu, Sep 11, 2014 at 07:56:31PM +0530, Sujith Manoharan wrote:
>> Rajkumar Manoharan wrote:
>>> Some of the IOE customer using ath9k with power save enabled. IIRC the
>>> expectation is that when the station is idle, it should wakeup on DTIM
>>> beacon not for every beacons. Are you seeing frequest bmiss events on
>>> higher beacon interval?
>>
>> I don't think ath9k has ever been tested with longer sleep durations
>> based on dtim period. We have always programmed the hardware to wake up
>> for every beacon interval, using the TIM_TIMER interrupt. And since the
>> TIM bit needs to be checked for every beacon, the sleep duration has
>> to be the beacon interval.
>>
>> Also, the bmiss threshold needs to be adjusted based on the sleep
>> duration. We are hardcoding it currently in the driver using
>> ATH_DEFAULT_BMISS_LIMIT. This will break, too.
>>
>> Using dtim for PS needs more testing before it can be used,
>> so we can have the original behavior for now.
>>
> Sounds clear.
> 
> Acked-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>

Just bisected that this patch caused kernel panic on ath9k_htc.
One more reason for revert ;)

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]