Re: [PATCH v2] mac80211: Fix a race on enabling power save.

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2011-02-04 at 17:55 +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/ieee80211_i.h |    1 +
>  net/mac80211/mlme.c        |    8 ++++++--
>  net/mac80211/tx.c          |    8 ++++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 533fd32..6ad97f6 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -346,6 +346,7 @@ enum ieee80211_sta_flags {
>  	IEEE80211_STA_UAPSD_ENABLED	= BIT(7),
>  	IEEE80211_STA_NULLFUNC_ACKED	= BIT(8),
>  	IEEE80211_STA_RESET_SIGNAL_AVE	= BIT(9),
> +	IEEE80211_STA_PS_PENDING	= BIT(10),
>  };
>  
>  struct ieee80211_if_managed {
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index e059b3a..45f736e 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -727,13 +727,17 @@ 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))) {
> +		ifmgd->flags |= IEEE80211_STA_PS_PENDING;
>  		ieee80211_send_nullfunc(local, sdata, 1);
> +	}
>  
>  	if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
>  	      (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
> -	    (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
> +	    ((ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) &&
> +	      ifmgd->flags & IEEE80211_STA_PS_PENDING))  {
>  		ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
> +		ifmgd->flags &= ~IEEE80211_STA_PS_PENDING;
>  		local->hw.conf.flags |= IEEE80211_CONF_PS;
>  		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
>  	}
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 8fbbc7a..e1c2256 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -185,6 +185,7 @@ ieee80211_tx_h_dynamic_ps(struct ieee80211_tx_data *tx)
>  {
>  	struct ieee80211_local *local = tx->local;
>  	struct ieee80211_if_managed *ifmgd;
> +	struct ieee80211_hdr *hdr;
>  
>  	/* driver doesn't support power save */
>  	if (!(local->hw.flags & IEEE80211_HW_SUPPORTS_PS))
> @@ -233,6 +234,13 @@ ieee80211_tx_h_dynamic_ps(struct ieee80211_tx_data *tx)
>  	    && skb_get_queue_mapping(tx->skb) == 0)
>  		return TX_CONTINUE;
>  
> +	hdr = (struct ieee80211_hdr *)tx->skb->data;
> +
> +	if (!(ieee80211_is_nullfunc(hdr->frame_control) &&
> +	     ieee80211_has_pm(hdr->frame_control)) &&
> +	    (ifmgd->flags & IEEE80211_STA_PS_PENDING))
> +		ifmgd->flags &= ~IEEE80211_STA_PS_PENDING;
> +
>  	if (local->hw.conf.flags & IEEE80211_CONF_PS) {

I don't see how this patch helps anything. Should the last line I quoted
be replaced instead by checking if PS was requested? We used to not wait
for the ACK -- so waiting for the ACK broke this.

johannes