* [PATCH 1/2] mac80211: Fix a race on enabling power save.
@ 2011-02-18 8:46 Vivek Natarajan
2011-02-18 11:07 ` Johannes Berg
0 siblings, 1 reply; 7+ messages in thread
From: Vivek Natarajan @ 2011-02-18 8:46 UTC (permalink / raw
To: linville; +Cc: linux-wireless, johannes
There is a race on sending a data frame before the tx completion
of nullfunc frame for enabling power save. As the data quickly
follows the nullfunc frame, the AP thinks that the station is out
of power save and continues to send the frames. Whereas in the
station, the nullfunc ack will be processed after the tx completion
of data frame and mac80211 goes to powersave. Thus the power
save state mismatch between the station and the AP causes some
data loss and some applications fail because of that. This patch
fixes this issue.
Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
---
net/mac80211/mlme.c | 11 ++++++++++-
net/mac80211/status.c | 2 --
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index d89e878..91006a2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
return;
if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
- (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
+ (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
+ netif_tx_stop_all_queues(sdata->dev);
+ /* flush all the frames queued in the driver before
+ * going to power save
+ */
+ drv_flush(local, false);
ieee80211_send_nullfunc(local, sdata, 1);
+ /* flush once again to get the tx status of nullfunc frame */
+ drv_flush(local, false);
+ }
if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
@@ -748,6 +756,7 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
local->hw.conf.flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
+ netif_tx_start_all_queues(sdata->dev);
}
void ieee80211_dynamic_ps_timer(unsigned long data)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 010a559..8651851 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -318,8 +318,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
if (info->flags & IEEE80211_TX_STAT_ACK) {
local->ps_sdata->u.mgd.flags |=
IEEE80211_STA_NULLFUNC_ACKED;
- ieee80211_queue_work(&local->hw,
- &local->dynamic_ps_enable_work);
} else
mod_timer(&local->dynamic_ps_timer, jiffies +
msecs_to_jiffies(10));
--
1.6.3.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-18 8:46 [PATCH 1/2] mac80211: Fix a race on enabling power save Vivek Natarajan
@ 2011-02-18 11:07 ` Johannes Berg
2011-02-19 6:12 ` Rajkumar Manoharan
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2011-02-18 11:07 UTC (permalink / raw
To: Vivek Natarajan; +Cc: linville, linux-wireless
On Fri, 2011-02-18 at 14:16 +0530, Vivek Natarajan wrote:
> There is a race on sending a data frame before the tx completion
> of nullfunc frame for enabling power save. As the data quickly
> follows the nullfunc frame, the AP thinks that the station is out
> of power save and continues to send the frames. Whereas in the
> station, the nullfunc ack will be processed after the tx completion
> of data frame and mac80211 goes to powersave. Thus the power
> save state mismatch between the station and the AP causes some
> data loss and some applications fail because of that. This patch
> fixes this issue.
>
> Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
> ---
> net/mac80211/mlme.c | 11 ++++++++++-
> net/mac80211/status.c | 2 --
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index d89e878..91006a2 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> return;
>
> if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> - (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
> + (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> + netif_tx_stop_all_queues(sdata->dev);
> + /* flush all the frames queued in the driver before
> + * going to power save
> + */
> + drv_flush(local, false);
Ok, I should've thought of this earlier -- but can we really do this?
I know we currently don't allow powersave when there's more than one
interface, but we still allow it while there are monitor interfaces and
a monitor interface could be sending packets, which would violate the
flush() contract with the driver -- the driver may expect not to get any
packets during flush.
I'm not sure. Maybe we should disable powersave when there are monitor
interfaces after all, even though that would rob us of a way to debug
things. Thoughts?
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-18 11:07 ` Johannes Berg
@ 2011-02-19 6:12 ` Rajkumar Manoharan
2011-02-19 9:14 ` Johannes Berg
0 siblings, 1 reply; 7+ messages in thread
From: Rajkumar Manoharan @ 2011-02-19 6:12 UTC (permalink / raw
To: Johannes Berg
Cc: Vivek Natarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
On Fri, Feb 18, 2011 at 04:37:25PM +0530, Johannes Berg wrote:
> On Fri, 2011-02-18 at 14:16 +0530, Vivek Natarajan wrote:
> > There is a race on sending a data frame before the tx completion
> > of nullfunc frame for enabling power save. As the data quickly
> > follows the nullfunc frame, the AP thinks that the station is out
> > of power save and continues to send the frames. Whereas in the
> > station, the nullfunc ack will be processed after the tx completion
> > of data frame and mac80211 goes to powersave. Thus the power
> > save state mismatch between the station and the AP causes some
> > data loss and some applications fail because of that. This patch
> > fixes this issue.
> >
> > Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
> > ---
> > net/mac80211/mlme.c | 11 ++++++++++-
> > net/mac80211/status.c | 2 --
> > 2 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index d89e878..91006a2 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> > return;
> >
> > if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> > - (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
> > + (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> > + netif_tx_stop_all_queues(sdata->dev);
> > + /* flush all the frames queued in the driver before
> > + * going to power save
> > + */
> > + drv_flush(local, false);
>
> Ok, I should've thought of this earlier -- but can we really do this?
>
> I know we currently don't allow powersave when there's more than one
> interface, but we still allow it while there are monitor interfaces and
> a monitor interface could be sending packets, which would violate the
> flush() contract with the driver -- the driver may expect not to get any
> packets during flush.
AFIK we also are not stopping tx queue of monitor iface on offchannel.
Doing so could help flush().
> I'm not sure. Maybe we should disable powersave when there are monitor
> interfaces after all, even though that would rob us of a way to debug
> things. Thoughts?
>
drivers like ath9k receives all frames from any bss on FIF_PROMISC_IN_BSS,
since there is no specific hw filter for that. So in that case it would be
better to disable power save.
--
Rajkumar
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-19 6:12 ` Rajkumar Manoharan
@ 2011-02-19 9:14 ` Johannes Berg
2011-02-21 7:21 ` Vivek Natarajan
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2011-02-19 9:14 UTC (permalink / raw
To: Rajkumar Manoharan
Cc: Vivek Natarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:
> > I know we currently don't allow powersave when there's more than one
> > interface, but we still allow it while there are monitor interfaces and
> > a monitor interface could be sending packets, which would violate the
> > flush() contract with the driver -- the driver may expect not to get any
> > packets during flush.
> AFIK we also are not stopping tx queue of monitor iface on offchannel.
> Doing so could help flush().
Hrm, indeed. And we can't even stop the queues there. We can pause them
for the flush though. In fact, why don't we just stop device queues for
flush and bypass this problem. I also think we should flush the status
tasklet when drv_flush() has returned so that all status processing is
also flushed out before we return to regular processing.
> > I'm not sure. Maybe we should disable powersave when there are monitor
> > interfaces after all, even though that would rob us of a way to debug
> > things. Thoughts?
> drivers like ath9k receives all frames from any bss on FIF_PROMISC_IN_BSS,
> since there is no specific hw filter for that. So in that case it would be
> better to disable power save.
Well, I'm not convinced yet -- it will likely also complicate the code
quite a bit when we want to implement NoA powersave etc. Of course we're
trying to get rid of monitor interfaces there anyway, but still ...
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-19 9:14 ` Johannes Berg
@ 2011-02-21 7:21 ` Vivek Natarajan
2011-02-22 13:02 ` Johannes Berg
0 siblings, 1 reply; 7+ messages in thread
From: Vivek Natarajan @ 2011-02-21 7:21 UTC (permalink / raw
To: Johannes Berg
Cc: Rajkumar Manoharan, Vivek Natarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
On Sat, Feb 19, 2011 at 2:44 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:
>
>> > I know we currently don't allow powersave when there's more than one
>> > interface, but we still allow it while there are monitor interfaces and
>> > a monitor interface could be sending packets, which would violate the
>> > flush() contract with the driver -- the driver may expect not to get any
>> > packets during flush.
>
>> AFIK we also are not stopping tx queue of monitor iface on offchannel.
>> Doing so could help flush().
>
> Hrm, indeed. And we can't even stop the queues there. We can pause them
> for the flush though. In fact, why don't we just stop device queues for
> flush and bypass this problem.
If the device queues are stopped, we have to find some other fix for
this power save issue since null func frame will be queued to pending
frames as I mentioned earlier. Can we have this patch merged and fix
the monitor vif issue for this case and the scanning case later? I
have also sent a RFC patch earlier with stopping device queues and a
new PS_PENDING flag for this
(http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
also addresses this race.
Vivek.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-21 7:21 ` Vivek Natarajan
@ 2011-02-22 13:02 ` Johannes Berg
2011-02-23 7:30 ` Vivek Natarajan
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2011-02-22 13:02 UTC (permalink / raw
To: Vivek Natarajan
Cc: Rajkumar Manoharan, Vivek Natarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
On Mon, 2011-02-21 at 12:51 +0530, Vivek Natarajan wrote:
> On Sat, Feb 19, 2011 at 2:44 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:
> >
> >> > I know we currently don't allow powersave when there's more than one
> >> > interface, but we still allow it while there are monitor interfaces and
> >> > a monitor interface could be sending packets, which would violate the
> >> > flush() contract with the driver -- the driver may expect not to get any
> >> > packets during flush.
> >
> >> AFIK we also are not stopping tx queue of monitor iface on offchannel.
> >> Doing so could help flush().
> >
> > Hrm, indeed. And we can't even stop the queues there. We can pause them
> > for the flush though. In fact, why don't we just stop device queues for
> > flush and bypass this problem.
>
> If the device queues are stopped, we have to find some other fix for
> this power save issue since null func frame will be queued to pending
> frames as I mentioned earlier. Can we have this patch merged and fix
> the monitor vif issue for this case and the scanning case later? I
> have also sent a RFC patch earlier with stopping device queues and a
> new PS_PENDING flag for this
> (http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
> also addresses this race.
I think for a first solution we can probably get away not worrying about
monitor injection frames, since typically they'd be used by P2P things
that disable powersave anyway. Right? And we already have the monitor
vs. flush problem anyway.
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.
2011-02-22 13:02 ` Johannes Berg
@ 2011-02-23 7:30 ` Vivek Natarajan
0 siblings, 0 replies; 7+ messages in thread
From: Vivek Natarajan @ 2011-02-23 7:30 UTC (permalink / raw
To: Johannes Berg
Cc: Rajkumar Manoharan, Vivek Natarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
On Tue, Feb 22, 2011 at 6:32 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> If the device queues are stopped, we have to find some other fix for
>> this power save issue since null func frame will be queued to pending
>> frames as I mentioned earlier. Can we have this patch merged and fix
>> the monitor vif issue for this case and the scanning case later? I
>> have also sent a RFC patch earlier with stopping device queues and a
>> new PS_PENDING flag for this
>> (http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
>> also addresses this race.
>
> I think for a first solution we can probably get away not worrying about
> monitor injection frames, since typically they'd be used by P2P things
> that disable powersave anyway. Right? And we already have the monitor
> vs. flush problem anyway.
Thanks. I will resend the patch rebased to current wireless-testing.
Vivek.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-23 7:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-18 8:46 [PATCH 1/2] mac80211: Fix a race on enabling power save Vivek Natarajan
2011-02-18 11:07 ` Johannes Berg
2011-02-19 6:12 ` Rajkumar Manoharan
2011-02-19 9:14 ` Johannes Berg
2011-02-21 7:21 ` Vivek Natarajan
2011-02-22 13:02 ` Johannes Berg
2011-02-23 7:30 ` Vivek Natarajan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).