Skip to contentSkip to conversation

mac80211: add power state transition callbacks

PATCH In review Christian Lamparter 2 participants 6 messages Nov 28, 2008 – Nov 29, 2008 mac80211

lore
Share & tools
lore

Christian Lamparter

author
#0 lore raw

PATCH 1/3 mac80211: add power state transition callbacks

This patch is necessary in order to provide a proper Access point support for p54.

Unfortunatly for us, there is no documented way to disable the interfering power save buffering mechanism in firmware completely.

Therefore we give in and notify the driver through our new sta_notify callback, so that we can update the filter state.

Signed-off-by Christian Lamparter <[email protected]>

---

include/net/mac80211.h +7 -3
index 6a1d4ea..7bd8edc 100644
@@ -768,14 +768,18 @@ struct ieee80211_sta {
 /**
  * enum sta_notify_cmd - sta notify command
  *
- * Used with the sta_notify() callback in &struct ieee80211_ops, this
- * indicates addition and removal of a station to station table.
+ * Used with the sta_notify() callback in &struct ieee80211_ops.
+ * this command indicates addition and removal of a station to
+ * station table, or if a station made a power state transition.
  *
  * @STA_NOTIFY_ADD: a station was added to the station table
  * @STA_NOTIFY_REMOVE: a station being removed from the station table
+ * @STA_NOTIFY_SLEEP: a station is now sleeping
+ * @STA_NOTIFY_AWAKE: a sleeping station woke up
  */
 enum sta_notify_cmd {
-	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE
+	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE,
+	STA_NOTIFY_SLEEP, STA_NOTIFY_AWAKE,
 };
 
 /**
net/mac80211/rx.c +7 -0
index 5a1a60f..2d311a1 100644
@@ -654,10 +654,14 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
 static void ap_sta_ps_start(struct sta_info *sta)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
+	struct ieee80211_local *local = sdata->local;
 	DECLARE_MAC_BUF(mac);
 
 	atomic_inc(&sdata->bss->num_sta_ps);
 	set_and_clear_sta_flags(sta, WLAN_STA_PS, WLAN_STA_PSPOLL);
+	if (local->ops->sta_notify)
+		local->ops->sta_notify(local_to_hw(local), &sdata->vif,
+				       STA_NOTIFY_SLEEP, &sta->sta);
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	printk(KERN_DEBUG "%s: STA %s aid %d enters power save mode\n",
 	       sdata->dev->name, print_mac(mac, sta->sta.addr), sta->sta.aid);

@@ -675,6 +679,9 @@ static int ap_sta_ps_end(struct sta_info *sta)
 	atomic_dec(&sdata->bss->num_sta_ps);
 
 	clear_sta_flags(sta, WLAN_STA_PS | WLAN_STA_PSPOLL);
+	if (local->ops->sta_notify)
+		local->ops->sta_notify(local_to_hw(local), &sdata->vif,
+				       STA_NOTIFY_AWAKE, &sta->sta);
 
 	if (!skb_queue_empty(&sta->ps_tx_buf))
 		sta_info_clear_tim_bit(sta);
maintainer
#1 lore raw
Quoted 15 lines · Christian Lamparter

/**
* enum sta_notify_cmd - sta notify command
*
- * Used with the sta_notify() callback in &struct ieee80211_ops, this
- * indicates addition and removal of a station to station table.
+ * Used with the sta_notify() callback in &struct ieee80211_ops.
+ * this command indicates addition and removal of a station to
+ * station table, or if a station made a power state transition. *
* @STA_NOTIFY_ADD: a station was added to the station table
* @STA_NOTIFY_REMOVE: a station being removed from the station table
+ * @STA_NOTIFY_SLEEP: a station is now sleeping
+ * @STA_NOTIFY_AWAKE: a sleeping station woke up
*/
enum sta_notify_cmd {

include/net/mac80211.h struct ieee80211_sta { · line 783

Quoted 4 lines
include/net/mac80211.h +2 -1
-	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE
+	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE,
+	STA_NOTIFY_SLEEP, STA_NOTIFY_AWAKE,
 };

You know, I'm sorry, but I've come to realise that adding it to sta_notify isn't a good idea after all. The one thing I forgot is that this new callback has to be atomic because it's called from the rx path, and sta_notify doesn't have to be.

Unless we want to defer RX packet processing to a workqueue rather than a tasklet?

johannes

author
#2 lore raw
Quoted 27 lines · On Saturday 29 November 2008 10:41:49 Johannes Berg wrote:
Quoted 15 lines · Christian Lamparter

/**
* enum sta_notify_cmd - sta notify command
*
- * Used with the sta_notify() callback in &struct ieee80211_ops, this
- * indicates addition and removal of a station to station table.
+ * Used with the sta_notify() callback in &struct ieee80211_ops.
+ * this command indicates addition and removal of a station to
+ * station table, or if a station made a power state transition. *
* @STA_NOTIFY_ADD: a station was added to the station table
* @STA_NOTIFY_REMOVE: a station being removed from the station table
+ * @STA_NOTIFY_SLEEP: a station is now sleeping
+ * @STA_NOTIFY_AWAKE: a sleeping station woke up
*/
enum sta_notify_cmd {

include/net/mac80211.h struct ieee80211_sta { · line 783

Quoted 4 lines
include/net/mac80211.h +2 -1
-	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE
+	STA_NOTIFY_ADD, STA_NOTIFY_REMOVE,
+	STA_NOTIFY_SLEEP, STA_NOTIFY_AWAKE,
 };

You know, I'm sorry, but I've come to realise that adding it to sta_notify isn't a good idea after all. The one thing I forgot is that this new callback has to be atomic because it's called from the rx path, and sta_notify doesn't have to be.

Quoted 3 lines

Unless we want to defer RX packet processing to a workqueue rather than a tasklet?

Hmm, but if we put the RX packet processing into a workqueue, we have to put the tx status report processing in one too (see comment about rx/tx race in main.c line 400)?

Or do you mean I should only put ap_sta_ps_end (& ap_sta_ps_start) resending stuff into a workqueue? (Yeah, this makes a lot more sense, and might actually work!, well let's prepare another patch)

Regards,
Chr

maintainer
#3 lore raw
Quoted 6 lines · Christian Lamparter
Quoted 2 lines

Unless we want to defer RX packet processing to a workqueue rather than a tasklet?

Hmm, but if we put the RX packet processing into a workqueue, we have to put the tx status report processing in one too (see comment about rx/tx race in main.c line 400)?

Yeah, probably. I don't know all the details all the time, so I write such comments ;)

Quoted 3 lines

Or do you mean I should only put ap_sta_ps_end (& ap_sta_ps_start) resending stuff into a workqueue? (Yeah, this makes a lot more sense, and might actually work!, well let's prepare another patch)

No idea!

The only thing I was thinking is that it's not a good plan to have a callback that has different locking requirements depending on a parameter.

johannes

author
#4 lore raw
Quoted 16 lines · On Saturday 29 November 2008 13:21:33 Johannes Berg wrote:
Quoted 6 lines · Christian Lamparter
Quoted 2 lines

Unless we want to defer RX packet processing to a workqueue rather than a tasklet?

Hmm, but if we put the RX packet processing into a workqueue, we have to put the tx status report processing in one too (see comment about rx/tx race in main.c line 400)?

Yeah, probably. I don't know all the details all the time, so I write such comments ;)

Quoted 3 lines

Or do you mean I should only put ap_sta_ps_end (& ap_sta_ps_start) resending stuff into a workqueue? (Yeah, this makes a lot more sense, and might actually work!, well let's prepare another patch)

Quoted 5 lines

No idea!

The only thing I was thinking is that it's not a good plan to have a callback that has different locking requirements depending on a parameter.

yeah... go back to the extra sta_notify_ps callback approach? I tried to do this with workqueues, but It turned out that queue_work doesn't work since we disabled the irqs in the rx path.
( http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-02/3350.html
has a story about the bitter consequences of queue_work & irq_disabled)

Regards,
Chr

maintainer
#5 lore raw
Quoted 5 lines · Christian Lamparter
Quoted 3 lines

The only thing I was thinking is that it's not a good plan to have a callback that has different locking requirements depending on a parameter.

yeah... go back to the extra sta_notify_ps callback approach?

Fine with me, yeah. It's not very complicated so we can always change it again later.

Quoted 4 lines

I tried to do this with workqueues, but It turned out that queue_work doesn't work since we disabled the irqs in the rx path.
( http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-02/3350.html
has a story about the bitter consequences of queue_work & irq_disabled)

Interesting.

johannes