[PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

When the driver has been notified with a STA_REMOVE, it tears down
the internal ADDBA state. On resume, trying to initiate aggregation would
fail because mac80211 has not cleared the operational state for that <TID,STA>.
This can be fixed by tearing down the existing sessions on a suspend.

Also, the driver can initiate a new BA session when suspend is in progress.
This is fixed by marking the station as being in suspend state and
denying ADDBA requests for such STAs.

Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
---
v2:
--
* Deny ADDBA requests if suspend is in progress.

 net/mac80211/agg-rx.c   |    8 ++++++++
 net/mac80211/agg-tx.c   |    9 +++++++++
 net/mac80211/pm.c       |   15 +++++++++++++++
 net/mac80211/sta_info.h |    3 +++
 4 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index a95affc..07656d8 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -197,6 +197,14 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 
 	status = WLAN_STATUS_REQUEST_DECLINED;
 
+	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		printk(KERN_DEBUG "Suspend in progress. "
+		       "Denying ADDBA request\n");
+#endif
+		goto end_no_lock;
+	}
+
 	/* sanity check for incoming parameters:
 	 * check if configuration can support the BA policy
 	 * and if buffer size does not exceeds max value */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 1df116d..e5776ef 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -257,6 +257,15 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
 		goto unlock;
 	}
 
+	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		printk(KERN_DEBUG "Suspend in progress. "
+		       "Denying BA session request\n");
+#endif
+		ret = -EINVAL;
+		goto unlock;
+	}
+
 	spin_lock_bh(&sta->lock);
 
 	sdata = sta->sdata;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index f845601..07929fb 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -21,6 +21,14 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 	list_for_each_entry(sdata, &local->interfaces, list)
 		ieee80211_disable_keys(sdata);
 
+	/* Tear down aggregation sessions */
+	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		list_for_each_entry(sta, &local->sta_list, list) {
+			set_sta_flags(sta, WLAN_STA_SUSPEND);
+			ieee80211_sta_tear_down_BA_sessions(sta);
+		}
+	}
+
 	/* remove STAs */
 	if (local->ops->sta_notify) {
 		spin_lock_irqsave(&local->sta_lock, flags);
@@ -102,6 +110,13 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
 		spin_unlock_irqrestore(&local->sta_lock, flags);
 	}
 
+	/* Clear Suspend state so that ADDBA requests can be processed */
+	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		list_for_each_entry(sta, &local->sta_list, list) {
+			clear_sta_flags(sta, WLAN_STA_SUSPEND);
+		}
+	}
+
 	/* add back keys */
 	list_for_each_entry(sdata, &local->interfaces, list)
 		if (netif_running(sdata->dev))
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1f45573..5b223b2 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -35,6 +35,8 @@
  *	IEEE80211_TX_CTL_CLEAR_PS_FILT control flag) when the next
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
+ * @WLAN_STA_SUSPEND: Set/cleared during a suspend/resume cycle.
+ *	Used to deny ADDBA requests (both TX and RX).
  */
 enum ieee80211_sta_info_flags {
 	WLAN_STA_AUTH		= 1<<0,
@@ -48,6 +50,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_PSPOLL		= 1<<8,
 	WLAN_STA_CLEAR_PS_FILT	= 1<<9,
 	WLAN_STA_MFP		= 1<<10,
+	WLAN_STA_SUSPEND	= 1<<11
 };
 
 #define STA_TID_NUM 16
-- 
1.6.2

[PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

When the driver has been notified with a STA_REMOVE, it tears down
the internal ADDBA state. On resume, trying to initiate aggregation would
fail because mac80211 has not cleared the operational state for that <TID,STA>.
This can be fixed by tearing down the existing sessions on a suspend.

Also, the driver can initiate a new BA session when suspend is in progress.
This is fixed by marking the station as being in suspend state and
denying ADDBA requests for such STAs.

Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
---

(SMTP borkage, try again)

v2:
--
* Deny ADDBA requests if suspend is in progress.

 net/mac80211/agg-rx.c   |    8 ++++++++
 net/mac80211/agg-tx.c   |    9 +++++++++
 net/mac80211/pm.c       |   15 +++++++++++++++
 net/mac80211/sta_info.h |    3 +++
 4 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index a95affc..07656d8 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -197,6 +197,14 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 
 	status = WLAN_STATUS_REQUEST_DECLINED;
 
+	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		printk(KERN_DEBUG "Suspend in progress. "
+		       "Denying ADDBA request\n");
+#endif
+		goto end_no_lock;
+	}
+
 	/* sanity check for incoming parameters:
 	 * check if configuration can support the BA policy
 	 * and if buffer size does not exceeds max value */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 1df116d..e5776ef 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -257,6 +257,15 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
 		goto unlock;
 	}
 
+	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		printk(KERN_DEBUG "Suspend in progress. "
+		       "Denying BA session request\n");
+#endif
+		ret = -EINVAL;
+		goto unlock;
+	}
+
 	spin_lock_bh(&sta->lock);
 
 	sdata = sta->sdata;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index f845601..07929fb 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -21,6 +21,14 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 	list_for_each_entry(sdata, &local->interfaces, list)
 		ieee80211_disable_keys(sdata);
 
+	/* Tear down aggregation sessions */
+	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		list_for_each_entry(sta, &local->sta_list, list) {
+			set_sta_flags(sta, WLAN_STA_SUSPEND);
+			ieee80211_sta_tear_down_BA_sessions(sta);
+		}
+	}
+
 	/* remove STAs */
 	if (local->ops->sta_notify) {
 		spin_lock_irqsave(&local->sta_lock, flags);
@@ -102,6 +110,13 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
 		spin_unlock_irqrestore(&local->sta_lock, flags);
 	}
 
+	/* Clear Suspend state so that ADDBA requests can be processed */
+	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		list_for_each_entry(sta, &local->sta_list, list) {
+			clear_sta_flags(sta, WLAN_STA_SUSPEND);
+		}
+	}
+
 	/* add back keys */
 	list_for_each_entry(sdata, &local->interfaces, list)
 		if (netif_running(sdata->dev))
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1f45573..5b223b2 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -35,6 +35,8 @@
  *	IEEE80211_TX_CTL_CLEAR_PS_FILT control flag) when the next
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
+ * @WLAN_STA_SUSPEND: Set/cleared during a suspend/resume cycle.
+ *	Used to deny ADDBA requests (both TX and RX).
  */
 enum ieee80211_sta_info_flags {
 	WLAN_STA_AUTH		= 1<<0,
@@ -48,6 +50,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_PSPOLL		= 1<<8,
 	WLAN_STA_CLEAR_PS_FILT	= 1<<9,
 	WLAN_STA_MFP		= 1<<10,
+	WLAN_STA_SUSPEND	= 1<<11
 };
 
 #define STA_TID_NUM 16
-- 
1.6.2

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Johannes Berg]

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

On Mon, 2009-03-16 at 15:20 +0530, Sujith wrote:
> When the driver has been notified with a STA_REMOVE, it tears down
> the internal ADDBA state. On resume, trying to initiate aggregation would
> fail because mac80211 has not cleared the operational state for that <TID,STA>.
> This can be fixed by tearing down the existing sessions on a suspend.
> 
> Also, the driver can initiate a new BA session when suspend is in progress.
> This is fixed by marking the station as being in suspend state and
> denying ADDBA requests for such STAs.
> 
> Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>

Looks good to me, thanks. Should we really set a per-station flag
though? It seems a "local->suspended" would be sufficient and much
cheaper?

johannes

> ---
> v2:
> --
> * Deny ADDBA requests if suspend is in progress.
> 
>  net/mac80211/agg-rx.c   |    8 ++++++++
>  net/mac80211/agg-tx.c   |    9 +++++++++
>  net/mac80211/pm.c       |   15 +++++++++++++++
>  net/mac80211/sta_info.h |    3 +++
>  4 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index a95affc..07656d8 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -197,6 +197,14 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
>  
>  	status = WLAN_STATUS_REQUEST_DECLINED;
>  
> +	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> +		printk(KERN_DEBUG "Suspend in progress. "
> +		       "Denying ADDBA request\n");
> +#endif
> +		goto end_no_lock;
> +	}
> +
>  	/* sanity check for incoming parameters:
>  	 * check if configuration can support the BA policy
>  	 * and if buffer size does not exceeds max value */
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
> index 1df116d..e5776ef 100644
> --- a/net/mac80211/agg-tx.c
> +++ b/net/mac80211/agg-tx.c
> @@ -257,6 +257,15 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
>  		goto unlock;
>  	}
>  
> +	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> +		printk(KERN_DEBUG "Suspend in progress. "
> +		       "Denying BA session request\n");
> +#endif
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
>  	spin_lock_bh(&sta->lock);
>  
>  	sdata = sta->sdata;
> diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
> index f845601..07929fb 100644
> --- a/net/mac80211/pm.c
> +++ b/net/mac80211/pm.c
> @@ -21,6 +21,14 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>  	list_for_each_entry(sdata, &local->interfaces, list)
>  		ieee80211_disable_keys(sdata);
>  
> +	/* Tear down aggregation sessions */
> +	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> +		list_for_each_entry(sta, &local->sta_list, list) {
> +			set_sta_flags(sta, WLAN_STA_SUSPEND);
> +			ieee80211_sta_tear_down_BA_sessions(sta);
> +		}
> +	}
> +
>  	/* remove STAs */
>  	if (local->ops->sta_notify) {
>  		spin_lock_irqsave(&local->sta_lock, flags);
> @@ -102,6 +110,13 @@ int __ieee80211_resume(struct ieee80211_hw *hw)
>  		spin_unlock_irqrestore(&local->sta_lock, flags);
>  	}
>  
> +	/* Clear Suspend state so that ADDBA requests can be processed */
> +	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> +		list_for_each_entry(sta, &local->sta_list, list) {
> +			clear_sta_flags(sta, WLAN_STA_SUSPEND);
> +		}
> +	}
> +
>  	/* add back keys */
>  	list_for_each_entry(sdata, &local->interfaces, list)
>  		if (netif_running(sdata->dev))
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 1f45573..5b223b2 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -35,6 +35,8 @@
>   *	IEEE80211_TX_CTL_CLEAR_PS_FILT control flag) when the next
>   *	frame to this station is transmitted.
>   * @WLAN_STA_MFP: Management frame protection is used with this STA.
> + * @WLAN_STA_SUSPEND: Set/cleared during a suspend/resume cycle.
> + *	Used to deny ADDBA requests (both TX and RX).
>   */
>  enum ieee80211_sta_info_flags {
>  	WLAN_STA_AUTH		= 1<<0,
> @@ -48,6 +50,7 @@ enum ieee80211_sta_info_flags {
>  	WLAN_STA_PSPOLL		= 1<<8,
>  	WLAN_STA_CLEAR_PS_FILT	= 1<<9,
>  	WLAN_STA_MFP		= 1<<10,
> +	WLAN_STA_SUSPEND	= 1<<11
>  };
>  
>  #define STA_TID_NUM 16

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Johannes Berg wrote:
> On Mon, 2009-03-16 at 15:20 +0530, Sujith wrote:
> > When the driver has been notified with a STA_REMOVE, it tears down
> > the internal ADDBA state. On resume, trying to initiate aggregation would
> > fail because mac80211 has not cleared the operational state for that <TID,STA>.
> > This can be fixed by tearing down the existing sessions on a suspend.
> > 
> > Also, the driver can initiate a new BA session when suspend is in progress.
> > This is fixed by marking the station as being in suspend state and
> > denying ADDBA requests for such STAs.
> > 
> > Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
> 
> Looks good to me, thanks. Should we really set a per-station flag
> though? It seems a "local->suspended" would be sufficient and much
> cheaper?

I did consider it, but then was unsure of the locking required.
If that can be clarified, I will change this to be a ieee80211_local variable.

Sujith

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Johannes Berg]

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

On Mon, 2009-03-16 at 16:04 +0530, Sujith wrote:
> Johannes Berg wrote:
> > On Mon, 2009-03-16 at 15:20 +0530, Sujith wrote:
> > > When the driver has been notified with a STA_REMOVE, it tears down
> > > the internal ADDBA state. On resume, trying to initiate aggregation would
> > > fail because mac80211 has not cleared the operational state for that <TID,STA>.
> > > This can be fixed by tearing down the existing sessions on a suspend.
> > > 
> > > Also, the driver can initiate a new BA session when suspend is in progress.
> > > This is fixed by marking the station as being in suspend state and
> > > denying ADDBA requests for such STAs.
> > > 
> > > Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
> > 
> > Looks good to me, thanks. Should we really set a per-station flag
> > though? It seems a "local->suspended" would be sufficient and much
> > cheaper?
> 
> I did consider it, but then was unsure of the locking required.
> If that can be clarified, I will change this to be a ieee80211_local variable.

Good catch, that does seem a little complicated. This way is good
anyway, so let's stick to it, at least for now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Johannes Berg]

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

On Mon, 2009-03-16 at 15:35 +0530, Sujith wrote:

> +	/* Tear down aggregation sessions */
> +	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> +		list_for_each_entry(sta, &local->sta_list, list) {
> +			set_sta_flags(sta, WLAN_STA_SUSPEND);
> +			ieee80211_sta_tear_down_BA_sessions(sta);
> +		}
> +	}

Doesn't that, and the corresponding code in resume, need to be using the
rcu-safe list iteration primitives and rcu_read_lock(), or acquire the
sta_lock like the other iteration? While we should be more or less
quiescent at this point, I wouldn't really want to bet on it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Johannes Berg wrote:
> Good catch, that does seem a little complicated. This way is good
> anyway, so let's stick to it, at least for now.

Ok.
Thanks for the review.

SUjith

Re: [PATCH v2] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Johannes Berg wrote:
> On Mon, 2009-03-16 at 15:35 +0530, Sujith wrote:
> 
> > +	/* Tear down aggregation sessions */
> > +	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> > +		list_for_each_entry(sta, &local->sta_list, list) {
> > +			set_sta_flags(sta, WLAN_STA_SUSPEND);
> > +			ieee80211_sta_tear_down_BA_sessions(sta);
> > +		}
> > +	}
> 
> Doesn't that, and the corresponding code in resume, need to be using the
> rcu-safe list iteration primitives and rcu_read_lock(), or acquire the
> sta_lock like the other iteration? While we should be more or less
> quiescent at this point, I wouldn't really want to bet on it.

Ah, right, it should be rcu-protected.
Will send out a v3 tomorrow.

Sujith