Re: [PATCH 09/12] ath9k: optimize ath9k_ps_restore

[Felix Fietkau <nbd@openwrt.org>]

On 2011-09-21 11:49 PM, Vivek Natarajan wrote:
> On Thu, Sep 15, 2011 at 12:54 AM, Felix Fietkau<nbd@openwrt.org>  wrote:
>>  ath_hw_cycle_counters_update only needs to be called if the power state
>>  changes. Most of the time this does not happen, even when ps_usecount
>>  goes down to 0.
>>
>>  Signed-off-by: Felix Fietkau<nbd@openwrt.org>
>>  ---
>>    drivers/net/wireless/ath/ath9k/main.c |   17 +++++++++++------
>>    1 files changed, 11 insertions(+), 6 deletions(-)
>>
>>  diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>>  index a75810a..a16f539 100644
>>  --- a/drivers/net/wireless/ath/ath9k/main.c
>>  +++ b/drivers/net/wireless/ath/ath9k/main.c
>>  @@ -111,24 +111,29 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
>>    void ath9k_ps_restore(struct ath_softc *sc)
>>    {
>>          struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>  +       enum ath9k_power_mode mode;
>>          unsigned long flags;
>>
>>          spin_lock_irqsave(&sc->sc_pm_lock, flags);
>>          if (--sc->ps_usecount != 0)
>>                  goto unlock;
>>
>>  -       spin_lock(&common->cc_lock);
>>  -       ath_hw_cycle_counters_update(common);
>>  -       spin_unlock(&common->cc_lock);
>>  -
>>          if (sc->ps_idle)
>>  -               ath9k_hw_setpower(sc->sc_ah, ATH9K_PM_FULL_SLEEP);
>>  +               mode = ATH9K_PM_FULL_SLEEP;
>>          else if (sc->ps_enabled&&
>>                   !(sc->ps_flags&  (PS_WAIT_FOR_BEACON |
>>                                PS_WAIT_FOR_CAB |
>>                                PS_WAIT_FOR_PSPOLL_DATA |
>>                                PS_WAIT_FOR_TX_ACK)))
>>  -               ath9k_hw_setpower(sc->sc_ah, ATH9K_PM_NETWORK_SLEEP);
>>  +               mode = ATH9K_PM_NETWORK_SLEEP;
>
> What is the use of setting the mode above if you use NETWORK_SLEEP by
> default in the below code.
Yes, I forgot to change it in the call below. I'll send a fix ASAP.

>>  +       else
>>  +               goto unlock;
>>  +
>>  +       spin_lock(&common->cc_lock);
>>  +       ath_hw_cycle_counters_update(common);
>>  +       spin_unlock(&common->cc_lock);
>>  +
>>  +       ath9k_hw_setpower(sc->sc_ah, ATH9K_PM_NETWORK_SLEEP);
>
> huh???
>
> It shows this patch was never tested and please clarify what you tried
> to achieve with this patch. Do you ever test a patch before sending it
> upstream?
>
> There is no wonder Johannes reports that power save is completely
> broken with ath9k if we have patches like these. We have better work
> to concentrate on, than to fix the regression your patch introduces
> every time.
>
> Please fix this and __test__ properly.
I did test this patch, and while it clearly has a bug (always setting 
network sleep instead of full sleep), I don't think this would break 
powersave. The only consequence I can think of is more battery drain 
when the card is idle and not connected.

What this patch achieves is this: ath9k_ps_restore is called frequently 
from functions handling the data path, also setting ps_usecount to 0 
frequently. That caused excessive calls to ath_hw_cycle_counters_update 
vasting a noticeable amount of CPU cycles (showed up during profiling).
My patch changes the code to only call ath_hw_cycle_counters_update 
before the card's power state changes.

- Felix