diff mbox

[RFC] mac80211: dynamic PS - don't enter PS when TX frames are pending

Message ID BANLkTimmn3o-+rexOYy2CjrOX_Xo06R+uw@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Vivek Natarajan June 21, 2011, 6:33 a.m. UTC 
On Tue, Jun 21, 2011 at 11:13 AM, Arik Nemtsov <arik@wizery.com> wrote:
> On Tue, Jun 21, 2011 at 07:55, Vivek Natarajan <vivek.natraj@gmail.com> wrote:
>>>
>>> +       /* don't enter PS if dynamic PS is enabled and TX frames are pending */
>>> +       if (local->hw.conf.dynamic_ps_timeout > 0 &&
>>> +           !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
>>
>> netif_tx_stop_all_queues has to be called before checking
>> frames_pending(), so that we can make sure no data is transmitted in
>> the time between checking frames_pending and transmitting null data
>> frames. If it is not done, this might cause some power save states
>> mismatch between the AP and the station in some corner cases.
>
> That's a good point. But I guess its only relevant to drivers using
> IEEE80211_HW_PS_NULLFUNC_STACK.
> Still seems racy to me - even if queues are stopped it doesn't mean
> mac80211 is not in the middle of ieee80211_tx(). Is there a way to
> make sure?

Well, I can't find any way to stop this race. :)
Atleast the other race could be fixed with stop_queues.
How about the below patch? I am not sure about the disable_dynamic_ps
and its BT/WLAN antenna sharing. Please add that if you see it as
appropriate.

                        drv_flush(local, false);


Vivek.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arik Nemtsov June 22, 2011, 8:58 a.m. UTC | #1 
On Tue, Jun 21, 2011 at 09:33, Vivek Natarajan <vivek.natraj@gmail.com> wrote:
>
> Well, I can't find any way to stop this race. :)
> Atleast the other race could be fixed with stop_queues.

Sure I'll add a stop_queues for for the IEEE80211_HW_PS_NULLFUNC_STACK
case. If you can't hit the other race I guess it doesn't matter. It's
a corner case anyway.

> How about the below patch? I am not sure about the disable_dynamic_ps
> and its BT/WLAN antenna sharing. Please add that if you see it as
> appropriate.

I think the disable_dynamic_ps and peers are needed. Otherwise we risk
getting inconsistent PS notifications from mac80211 when dyn_ps is
disabled.

>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index faca503..67ecad1 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -778,15 +778,14 @@ void ieee80211_dynamic_ps_enable_work(struct
> work_struct *work)
>        }
>        spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>
> -       if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> -           (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> +       if (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
>                netif_tx_stop_all_queues(sdata->dev);
>
>                if (drv_tx_frames_pending(local))
>                        mod_timer(&local->dynamic_ps_timer, jiffies +
>                                  msecs_to_jiffies(
>                                  local->hw.conf.dynamic_ps_timeout));
> -               else {
> +               else if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) {
>                        ieee80211_send_nullfunc(local, sdata, 1);
>                        /* Flush to get the tx status of nullfunc frame */
>                        drv_flush(local, false);
>

Well drivers without IEEE80211_HW_PS_NULLFUNC_STACK need to leave the
function after mod_timer(). Otherwise we end up changing the PS state
anyway.
But yea, basically its very similar to my patch.

I'll submit a v2 with the stop_queues pending Sam's review.

Arik
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index faca503..67ecad1 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -778,15 +778,14 @@  void ieee80211_dynamic_ps_enable_work(struct
work_struct *work)
        }
        spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);

-       if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
-           (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
+       if (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
                netif_tx_stop_all_queues(sdata->dev);

                if (drv_tx_frames_pending(local))
                        mod_timer(&local->dynamic_ps_timer, jiffies +
                                  msecs_to_jiffies(
                                  local->hw.conf.dynamic_ps_timeout));
-               else {
+               else if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) {
                        ieee80211_send_nullfunc(local, sdata, 1);
                        /* Flush to get the tx status of nullfunc frame */