[PATCH 00/10] wl12xx: AP-mode per link PSM

[PATCH 00/10] wl12xx: AP-mode per link PSM

[Arik Nemtsov]

This patchset introduces per-link PSM for AP-mode. The PS status of each
link is updated from FW.
The wl12xx concept of PSM is different from the mac80211 one - some data
must remain in FW for a link in PSM since the TIM is updated automatically
by FW.
Exported functions to manually toggle PSM in mac80211 are added
to bridge this gap.

We set up a skb-queue per link which allows for a cleaner link
disconnection and prepares the code for regulating each link separately.

A few other AP-mode related small bug fixes and enchancements are included.

Arik Nemtsov (10):
  wl12xx: fix potential race condition with TX queue watermark
  wl12xx: AP-mode - fix race condition on sta connection
  wl12xx: AP-mode - TX queue per link in AC
  mac80211: do not calc frame duration when using HW rate-control
  wl12xx: report invalid TX rate when returning non-TX-ed skbs
  mac80211: add HW flag for disabling auto link-PS in AP mode
  wl12xx: AP-mode - support HW based link PS monitoring
  wl12xx: AP mode - fix bug in cleanup of wl1271_op_sta_add()
  wl12xx: AP-mode - count free FW TX blocks per link
  wl12xx: AP-mode - management of links in PS-mode

 drivers/net/wireless/wl12xx/acx.c    |   25 ++++
 drivers/net/wireless/wl12xx/acx.h    |    9 ++
 drivers/net/wireless/wl12xx/main.c   |  125 +++++++++++++++----
 drivers/net/wireless/wl12xx/ps.c     |   78 ++++++++++++
 drivers/net/wireless/wl12xx/ps.h     |    2 +
 drivers/net/wireless/wl12xx/tx.c     |  232 +++++++++++++++++++++++++++++-----
 drivers/net/wireless/wl12xx/tx.h     |    3 +
 drivers/net/wireless/wl12xx/wl12xx.h |   37 ++++++
 include/net/mac80211.h               |   38 ++++++
 net/mac80211/rx.c                    |   19 +++-
 net/mac80211/tx.c                    |    3 +-
 11 files changed, 512 insertions(+), 59 deletions(-)

[PATCH 01/10] wl12xx: fix potential race condition with TX queue watermark

[Arik Nemtsov]

Check the conditions for the high/low TX queue watermarks when the
spin-lock is taken. This prevents race conditions as tx_queue_count and
the flag checked are only modified when the spin-lock is taken.

The following race was in mind:
- Queues are almost full and wl1271_op_tx() will stop the queues, but it
  doesn't get the spin-lock yet.
- (on another CPU) tx_work_locked() dequeues 15 skbs from this queue and
  tx_queue_count is updated to reflect this
- wl1271_op_tx() does not check tx_queue_count after taking the
  spin-lock and incorrectly stops the queue.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |   24 +++++++++++-------------
 drivers/net/wireless/wl12xx/tx.c   |    2 +-
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 9076555..6163167 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -984,6 +984,17 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	}
 #endif
 	wl->tx_queue_count++;
+
+	/*
+	 * The workqueue is slow to process the tx_queue and we need stop
+	 * the queue here, otherwise the queue will get too long.
+	 */
+	if (wl->tx_queue_count >= WL1271_TX_QUEUE_HIGH_WATERMARK) {
+		wl1271_debug(DEBUG_TX, "op_tx: stopping queues");
+		ieee80211_stop_queues(wl->hw);
+		__set_bit(WL1271_FLAG_TX_QUEUE_STOPPED, &wl->flags);
+	}
+
 	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
 	/* queue the packet */
@@ -998,19 +1009,6 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (!test_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags))
 		ieee80211_queue_work(wl->hw, &wl->tx_work);
 
-	/*
-	 * The workqueue is slow to process the tx_queue and we need stop
-	 * the queue here, otherwise the queue will get too long.
-	 */
-	if (wl->tx_queue_count >= WL1271_TX_QUEUE_HIGH_WATERMARK) {
-		wl1271_debug(DEBUG_TX, "op_tx: stopping queues");
-
-		spin_lock_irqsave(&wl->wl_lock, flags);
-		ieee80211_stop_queues(wl->hw);
-		set_bit(WL1271_FLAG_TX_QUEUE_STOPPED, &wl->flags);
-		spin_unlock_irqrestore(&wl->wl_lock, flags);
-	}
-
 	return NETDEV_TX_OK;
 }
 
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index 2347f25..fdec4a3 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -288,7 +288,7 @@ static void handle_tx_low_watermark(struct wl1271 *wl)
 		/* firmware buffer has space, restart queues */
 		spin_lock_irqsave(&wl->wl_lock, flags);
 		ieee80211_wake_queues(wl->hw);
-		clear_bit(WL1271_FLAG_TX_QUEUE_STOPPED, &wl->flags);
+		__clear_bit(WL1271_FLAG_TX_QUEUE_STOPPED, &wl->flags);
 		spin_unlock_irqrestore(&wl->wl_lock, flags);
 	}
 }
-- 
1.7.1

[PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Arik Nemtsov]

If a sta starts transmitting immediately after authentication, sometimes
the FW deauthenticates it. Fix this by marking the sta "in-connection"
in FW before sending the autentication response.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/acx.c |   25 +++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/acx.h |    9 +++++++++
 drivers/net/wireless/wl12xx/tx.c  |   19 +++++++++++++++++++
 3 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index 646d278..1b5a371 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -1391,3 +1391,28 @@ out:
 	kfree(acx);
 	return ret;
 }
+
+int wl1271_acx_set_inconnection_sta(struct wl1271 *wl, u8 *addr)
+{
+	struct wl1271_acx_inconnection_sta *acx = NULL;
+	int ret;
+
+	wl1271_debug(DEBUG_ACX, "acx set inconnaction sta %pM", addr);
+
+	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+	if (!acx)
+		return -ENOMEM;
+
+	memcpy(acx->addr, addr, ETH_ALEN);
+
+	ret = wl1271_cmd_configure(wl, ACX_UPDATE_INCONNECTION_STA_LIST,
+				   acx, sizeof(*acx));
+	if (ret < 0) {
+		wl1271_warning("acx set inconnaction sta failed: %d", ret);
+		goto out;
+	}
+
+out:
+	kfree(acx);
+	return ret;
+}
diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
index 89d3748..4ea3378 100644
--- a/drivers/net/wireless/wl12xx/acx.h
+++ b/drivers/net/wireless/wl12xx/acx.h
@@ -1083,6 +1083,13 @@ struct wl1271_acx_max_tx_retry {
 	u8 padding_1[2];
 } __packed;
 
+struct wl1271_acx_inconnection_sta {
+	struct acx_header header;
+
+	u8 addr[ETH_ALEN];
+	u8 padding1[2];
+} __packed;
+
 enum {
 	ACX_WAKE_UP_CONDITIONS      = 0x0002,
 	ACX_MEM_CFG                 = 0x0003,
@@ -1141,6 +1148,7 @@ enum {
 	ACX_COEX_ACTIVITY           = 0x0059,
 	ACX_SET_DCO_ITRIM_PARAMS    = 0x0061,
 	ACX_MAX_TX_FAILURE          = 0x0072,
+	ACX_UPDATE_INCONNECTION_STA_LIST = 0x0073,
 	DOT11_RX_MSDU_LIFE_TIME     = 0x1004,
 	DOT11_CUR_TX_PWR            = 0x100D,
 	DOT11_RX_DOT11_MODE         = 0x1012,
@@ -1211,5 +1219,6 @@ int wl1271_acx_set_ht_information(struct wl1271 *wl,
 				   u16 ht_operation_mode);
 int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime);
 int wl1271_acx_max_tx_retry(struct wl1271 *wl);
+int wl1271_acx_set_inconnection_sta(struct wl1271 *wl, u8 *addr);
 
 #endif /* __WL1271_ACX_H__ */
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index fdec4a3..d3ba27c 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -70,6 +70,22 @@ static void wl1271_free_tx_id(struct wl1271 *wl, int id)
 	}
 }
 
+static void wl1271_tx_ap_update_inconnection_sta(struct wl1271 *wl,
+					      struct sk_buff *skb)
+{
+	struct ieee80211_hdr *hdr;
+
+	/*
+	 * add the station to the known list before broadcasting the
+	 * authentication response. this way it won't get de-authed by FW
+	 * when transmitting too soon.
+	 */
+	hdr = (struct ieee80211_hdr *)(skb->data +
+				       sizeof(struct wl1271_tx_hw_descr));
+	if (ieee80211_is_auth(hdr->frame_control))
+		wl1271_acx_set_inconnection_sta(wl, hdr->addr1);
+}
+
 static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
 				u32 buf_offset)
 {
@@ -238,6 +254,9 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
 	if (ret < 0)
 		return ret;
 
+	if (wl->bss_type == BSS_TYPE_AP_BSS)
+		wl1271_tx_ap_update_inconnection_sta(wl, skb);
+
 	wl1271_tx_fill_hdr(wl, skb, extra, info);
 
 	/*
-- 
1.7.1

[PATCH 03/10] wl12xx: AP-mode - TX queue per link in AC

[Arik Nemtsov]

When operating in AP-mode we require a per link tx-queue.
This allows us to implement HW assisted PS mode for links,
as well as regulate per-link FW TX blocks consumption.
Split each link into ACs to support future QoS for AP-mode.

AC queues are emptied in priority and per-link queues are
scheduled in a simple round-robin fashion.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c   |   17 ++++-
 drivers/net/wireless/wl12xx/tx.c     |  130 +++++++++++++++++++++++++++++++---
 drivers/net/wireless/wl12xx/tx.h     |    3 +
 drivers/net/wireless/wl12xx/wl12xx.h |   14 ++++
 4 files changed, 151 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 6163167..6318224 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -955,6 +955,7 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	struct ieee80211_sta *sta = txinfo->control.sta;
 	unsigned long flags;
 	int q;
+	u8 hlid = 0;
 
 	/*
 	 * peek into the rates configured in the STA entry.
@@ -999,7 +1000,13 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 
 	/* queue the packet */
 	q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
-	skb_queue_tail(&wl->tx_queue[q], skb);
+	if (wl->bss_type == BSS_TYPE_AP_BSS) {
+		hlid = wl1271_tx_get_hlid(skb);
+		wl1271_debug(DEBUG_TX, "queue skb hlid %d q %d", hlid, q);
+		skb_queue_tail(&wl->links[hlid].tx_queue[q], skb);
+	} else {
+		skb_queue_tail(&wl->tx_queue[q], skb);
+	}
 
 	/*
 	 * The chip specific setup must run before the first TX packet -
@@ -2630,6 +2637,7 @@ static void wl1271_free_hlid(struct wl1271 *wl, u8 hlid)
 	int id = hlid - WL1271_AP_STA_HLID_START;
 
 	__clear_bit(id, wl->ap_hlid_map);
+	wl1271_tx_reset_link_queues(wl, hlid);
 }
 
 static int wl1271_op_sta_add(struct ieee80211_hw *hw,
@@ -3189,7 +3197,7 @@ struct ieee80211_hw *wl1271_alloc_hw(void)
 	struct ieee80211_hw *hw;
 	struct platform_device *plat_dev = NULL;
 	struct wl1271 *wl;
-	int i, ret;
+	int i, j, ret;
 	unsigned int order;
 
 	hw = ieee80211_alloc_hw(sizeof(*wl), &wl1271_ops);
@@ -3217,6 +3225,10 @@ struct ieee80211_hw *wl1271_alloc_hw(void)
 	for (i = 0; i < NUM_TX_QUEUES; i++)
 		skb_queue_head_init(&wl->tx_queue[i]);
 
+	for (i = 0; i < NUM_TX_QUEUES; i++)
+		for (j = 0; j < AP_MAX_LINKS; j++)
+			skb_queue_head_init(&wl->links[j].tx_queue[i]);
+
 	INIT_DELAYED_WORK(&wl->elp_work, wl1271_elp_work);
 	INIT_DELAYED_WORK(&wl->pspoll_work, wl1271_pspoll_work);
 	INIT_WORK(&wl->irq_work, wl1271_irq_work);
@@ -3243,6 +3255,7 @@ struct ieee80211_hw *wl1271_alloc_hw(void)
 	wl->bss_type = MAX_BSS_TYPE;
 	wl->set_bss_type = MAX_BSS_TYPE;
 	wl->fw_bss_type = MAX_BSS_TYPE;
+	wl->last_tx_hlid = 0;
 
 	memset(wl->tx_frames_map, 0, sizeof(wl->tx_frames_map));
 	for (i = 0; i < ACX_TX_DESCRIPTORS; i++)
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index d3ba27c..b7fd840 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -86,6 +86,27 @@ static void wl1271_tx_ap_update_inconnection_sta(struct wl1271 *wl,
 		wl1271_acx_set_inconnection_sta(wl, hdr->addr1);
 }
 
+u8 wl1271_tx_get_hlid(struct sk_buff *skb)
+{
+	struct ieee80211_tx_info *control = IEEE80211_SKB_CB(skb);
+
+	if (control->control.sta) {
+		struct wl1271_station *wl_sta;
+
+		wl_sta = (struct wl1271_station *)
+				control->control.sta->drv_priv;
+		return wl_sta->hlid;
+	} else {
+		struct ieee80211_hdr *hdr;
+
+		hdr = (struct ieee80211_hdr *)skb->data;
+		if (ieee80211_is_mgmt(hdr->frame_control))
+			return WL1271_AP_GLOBAL_HLID;
+		else
+			return WL1271_AP_BROADCAST_HLID;
+	}
+}
+
 static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
 				u32 buf_offset)
 {
@@ -298,7 +319,7 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set)
 	return enabled_rates;
 }
 
-static void handle_tx_low_watermark(struct wl1271 *wl)
+void wl1271_handle_tx_low_watermark(struct wl1271 *wl)
 {
 	unsigned long flags;
 
@@ -312,7 +333,7 @@ static void handle_tx_low_watermark(struct wl1271 *wl)
 	}
 }
 
-static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
+static struct sk_buff *wl1271_sta_skb_dequeue(struct wl1271 *wl)
 {
 	struct sk_buff *skb = NULL;
 	unsigned long flags;
@@ -338,12 +359,69 @@ out:
 	return skb;
 }
 
+static struct sk_buff *wl1271_ap_skb_dequeue(struct wl1271 *wl)
+{
+	struct sk_buff *skb = NULL;
+	unsigned long flags;
+	int i, h, start_hlid;
+
+	/* start from the link after the last one */
+	start_hlid = (wl->last_tx_hlid + 1) % AP_MAX_LINKS;
+
+	/* dequeue according to AC, round robin on each link */
+	for (i = 0; i < AP_MAX_LINKS; i++) {
+		h = (start_hlid + i) % AP_MAX_LINKS;
+
+		skb = skb_dequeue(&wl->links[h].tx_queue[CONF_TX_AC_VO]);
+		if (skb)
+			goto out;
+		skb = skb_dequeue(&wl->links[h].tx_queue[CONF_TX_AC_VI]);
+		if (skb)
+			goto out;
+		skb = skb_dequeue(&wl->links[h].tx_queue[CONF_TX_AC_BE]);
+		if (skb)
+			goto out;
+		skb = skb_dequeue(&wl->links[h].tx_queue[CONF_TX_AC_BK]);
+		if (skb)
+			goto out;
+	}
+
+out:
+	if (skb) {
+		wl->last_tx_hlid = h;
+		spin_lock_irqsave(&wl->wl_lock, flags);
+		wl->tx_queue_count--;
+		spin_unlock_irqrestore(&wl->wl_lock, flags);
+	} else {
+		wl->last_tx_hlid = 0;
+	}
+
+	return skb;
+}
+
+static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
+{
+	if (wl->bss_type == BSS_TYPE_AP_BSS)
+		return wl1271_ap_skb_dequeue(wl);
+
+	return wl1271_sta_skb_dequeue(wl);
+}
+
 static void wl1271_skb_queue_head(struct wl1271 *wl, struct sk_buff *skb)
 {
 	unsigned long flags;
 	int q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
 
-	skb_queue_head(&wl->tx_queue[q], skb);
+	if (wl->bss_type == BSS_TYPE_AP_BSS) {
+		u8 hlid = wl1271_tx_get_hlid(skb);
+		skb_queue_head(&wl->links[hlid].tx_queue[q], skb);
+
+		/* make sure we dequeue the same packet next time */
+		wl->last_tx_hlid = (hlid + AP_MAX_LINKS - 1) % AP_MAX_LINKS;
+	} else {
+		skb_queue_head(&wl->tx_queue[q], skb);
+	}
+
 	spin_lock_irqsave(&wl->wl_lock, flags);
 	wl->tx_queue_count++;
 	spin_unlock_irqrestore(&wl->wl_lock, flags);
@@ -428,7 +506,7 @@ out_ack:
 	if (sent_packets) {
 		/* interrupt the firmware with the new packets */
 		wl1271_write32(wl, WL1271_HOST_WR_ACCESS, wl->tx_packets_count);
-		handle_tx_low_watermark(wl);
+		wl1271_handle_tx_low_watermark(wl);
 	}
 
 out:
@@ -545,6 +623,27 @@ void wl1271_tx_complete(struct wl1271 *wl)
 	}
 }
 
+void wl1271_tx_reset_link_queues(struct wl1271 *wl, u8 hlid)
+{
+	struct sk_buff *skb;
+	int i, total = 0;
+	unsigned long flags;
+
+	for (i = 0; i < NUM_TX_QUEUES; i++) {
+		while ((skb = skb_dequeue(&wl->links[hlid].tx_queue[i]))) {
+			wl1271_debug(DEBUG_TX, "link freeing skb 0x%p", skb);
+			ieee80211_tx_status(wl->hw, skb);
+			total++;
+		}
+	}
+
+	spin_lock_irqsave(&wl->wl_lock, flags);
+	wl->tx_queue_count -= total;
+	spin_unlock_irqrestore(&wl->wl_lock, flags);
+
+	wl1271_handle_tx_low_watermark(wl);
+}
+
 /* caller must hold wl->mutex */
 void wl1271_tx_reset(struct wl1271 *wl)
 {
@@ -552,19 +651,28 @@ void wl1271_tx_reset(struct wl1271 *wl)
 	struct sk_buff *skb;
 
 	/* TX failure */
-	for (i = 0; i < NUM_TX_QUEUES; i++) {
-		while ((skb = skb_dequeue(&wl->tx_queue[i]))) {
-			wl1271_debug(DEBUG_TX, "freeing skb 0x%p", skb);
-			ieee80211_tx_status(wl->hw, skb);
+	if (wl->bss_type == BSS_TYPE_AP_BSS) {
+		for (i = 0; i < AP_MAX_LINKS; i++)
+			wl1271_tx_reset_link_queues(wl, i);
+
+		wl->last_tx_hlid = 0;
+	} else {
+		for (i = 0; i < NUM_TX_QUEUES; i++) {
+			while ((skb = skb_dequeue(&wl->tx_queue[i]))) {
+				wl1271_debug(DEBUG_TX, "freeing skb 0x%p",
+					     skb);
+				ieee80211_tx_status(wl->hw, skb);
+			}
 		}
 	}
+
 	wl->tx_queue_count = 0;
 
 	/*
 	 * Make sure the driver is at a consistent state, in case this
 	 * function is called from a context other than interface removal.
 	 */
-	handle_tx_low_watermark(wl);
+	wl1271_handle_tx_low_watermark(wl);
 
 	for (i = 0; i < ACX_TX_DESCRIPTORS; i++)
 		if (wl->tx_frames[i] != NULL) {
@@ -585,8 +693,8 @@ void wl1271_tx_flush(struct wl1271 *wl)
 
 	while (!time_after(jiffies, timeout)) {
 		mutex_lock(&wl->mutex);
-		wl1271_debug(DEBUG_TX, "flushing tx buffer: %d",
-			     wl->tx_frames_cnt);
+		wl1271_debug(DEBUG_TX, "flushing tx buffer: %d %d",
+			     wl->tx_frames_cnt, wl->tx_queue_count);
 		if ((wl->tx_frames_cnt == 0) && (wl->tx_queue_count == 0)) {
 			mutex_unlock(&wl->mutex);
 			return;
diff --git a/drivers/net/wireless/wl12xx/tx.h b/drivers/net/wireless/wl12xx/tx.h
index 05722a5..76df5d5 100644
--- a/drivers/net/wireless/wl12xx/tx.h
+++ b/drivers/net/wireless/wl12xx/tx.h
@@ -152,5 +152,8 @@ void wl1271_tx_flush(struct wl1271 *wl);
 u8 wl1271_rate_to_idx(int rate, enum ieee80211_band band);
 u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set);
 u32 wl1271_tx_min_rate_get(struct wl1271 *wl);
+u8 wl1271_tx_get_hlid(struct sk_buff *skb);
+void wl1271_tx_reset_link_queues(struct wl1271 *wl, u8 hlid);
+void wl1271_handle_tx_low_watermark(struct wl1271 *wl);
 
 #endif
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index cf5c1a5..0027e00 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -268,6 +268,11 @@ struct wl1271_ap_key {
 	u16 tx_seq_16;
 };
 
+struct wl1271_link {
+	/* AP-mode - TX queue per AC in link */
+	struct sk_buff_head tx_queue[NUM_TX_QUEUES];
+};
+
 struct wl1271 {
 	struct platform_device *plat_dev;
 	struct ieee80211_hw *hw;
@@ -460,6 +465,15 @@ struct wl1271 {
 
 	/* bands supported by this instance of wl12xx */
 	struct ieee80211_supported_band bands[IEEE80211_NUM_BANDS];
+
+	/*
+	 * AP-mode - links indexed by HLID. The global and broadcast links
+	 * are always active.
+	 */
+	struct wl1271_link links[AP_MAX_LINKS];
+
+	/* the hlid of the link where the last transmitted skb came from */
+	int last_tx_hlid;
 };
 
 struct wl1271_station {
-- 
1.7.1

[PATCH 04/10] mac80211: do not calc frame duration when using HW rate-control

[Arik Nemtsov]

When rate-control is performed in HW, we cannot calculate frame
duration as we do not have the skb transmission rate in SW.

ieee80211_tx_h_calculate_duration() should only be called when
ieee80211_tx_h_rate_ctrl() has been called before to initialize data
in skb->cb. This doesn't happen for drivers with HW rate-control.

Fixes the following warning when operating in AP-mode
in a driver with HW rate-control.

<4>[16747.586029] WARNING: at net/mac80211/tx.c:57 ieee80211_duration
+0x54/0x1d8 [mac80211]()
<4>[16747.594085] Modules linked in: wl12xx_sdio wl12xx firmware_class crc7
mac80211 cfg80211 [last unloaded: crc7]
<4>[16747.604095] [<c004707c>] (unwind_backtrace+0x0/0x124) from [<c006644c>]
(warn_slowpath_common+0x4c/0x64)
<4>[16747.613616] [<c006644c>] (warn_slowpath_common+0x4c/0x64) from
[<c006647c>] (warn_slowpath_null+0x18/0x1c)
<4>[16747.623382] [<c006647c>] (warn_slowpath_null+0x18/0x1c) from [<bf1aa8e8>]
(ieee80211_duration+0x54/0x1d8 [mac80211])
<4>[16747.634124] [<bf1aa8e8>] (ieee80211_duration+0x54/0x1d8 [mac80211]) from
[<bf1aacec>] (ieee80211_tx_h_calculate_duration+0x3c/0x5c [mac80211])
<4>[16747.647094] [<bf1aacec>] (ieee80211_tx_h_calculate_duration+0x3c/0x5c
[mac80211]) from [<bf1ab21c>] (invoke_tx_handlers+0xdc/0x118 [mac80211])
<4>[16747.660064] [<bf1ab21c>] (invoke_tx_handlers+0xdc/0x118 [mac80211])
from [<bf1ab2dc>] (ieee80211_tx+0x84/0x248 [mac80211])
<4>[16747.671295] [<bf1ab2dc>] (ieee80211_tx+0x84/0x248 [mac80211]) from
[<bf1abe3c>] (ieee80211_tx_pending+0x12c/0x278 [mac80211])
<4>[16747.682739] [<bf1abe3c>] (ieee80211_tx_pending+0x12c/0x278 [mac80211])
from [<c006b3bc>] (tasklet_action+0x68/0xc0)

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 net/mac80211/tx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5950e3a..e0d6d5c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1394,7 +1394,8 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
 	/* handlers after fragment must be aware of tx info fragmentation! */
 	CALL_TXH(ieee80211_tx_h_stats);
 	CALL_TXH(ieee80211_tx_h_encrypt);
-	CALL_TXH(ieee80211_tx_h_calculate_duration);
+	if (!(tx->local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL))
+		CALL_TXH(ieee80211_tx_h_calculate_duration);
 #undef CALL_TXH
 
  txh_done:
-- 
1.7.1

[PATCH 05/10] wl12xx: report invalid TX rate when returning non-TX-ed skbs

[Arik Nemtsov]

Report a TX rate idx of -1 when returning untransmitted skbs to mac80211
using ieee80211_tx_status(). Otherwise mac80211 tries to use the
returned (essentially garbage) status.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/tx.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index b7fd840..d43193e 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -628,10 +628,13 @@ void wl1271_tx_reset_link_queues(struct wl1271 *wl, u8 hlid)
 	struct sk_buff *skb;
 	int i, total = 0;
 	unsigned long flags;
+	struct ieee80211_tx_info *info;
 
 	for (i = 0; i < NUM_TX_QUEUES; i++) {
 		while ((skb = skb_dequeue(&wl->links[hlid].tx_queue[i]))) {
 			wl1271_debug(DEBUG_TX, "link freeing skb 0x%p", skb);
+			info = IEEE80211_SKB_CB(skb);
+			info->status.rates[0].idx = -1;
 			ieee80211_tx_status(wl->hw, skb);
 			total++;
 		}
@@ -649,6 +652,7 @@ void wl1271_tx_reset(struct wl1271 *wl)
 {
 	int i;
 	struct sk_buff *skb;
+	struct ieee80211_tx_info *info;
 
 	/* TX failure */
 	if (wl->bss_type == BSS_TYPE_AP_BSS) {
@@ -661,6 +665,8 @@ void wl1271_tx_reset(struct wl1271 *wl)
 			while ((skb = skb_dequeue(&wl->tx_queue[i]))) {
 				wl1271_debug(DEBUG_TX, "freeing skb 0x%p",
 					     skb);
+				info = IEEE80211_SKB_CB(skb);
+				info->status.rates[0].idx = -1;
 				ieee80211_tx_status(wl->hw, skb);
 			}
 		}
@@ -679,6 +685,8 @@ void wl1271_tx_reset(struct wl1271 *wl)
 			skb = wl->tx_frames[i];
 			wl1271_free_tx_id(wl, i);
 			wl1271_debug(DEBUG_TX, "freeing skb 0x%p", skb);
+			info = IEEE80211_SKB_CB(skb);
+			info->status.rates[0].idx = -1;
 			ieee80211_tx_status(wl->hw, skb);
 		}
 }
-- 
1.7.1

[PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Arik Nemtsov]

When operating in AP mode the wl1271 hardware filters our null-data
packets as well as management packets. This makes it impossible for
mac80211 to monitor the PS mode by using the PM bit of incoming frames.

Implement a HW flag to indicate that mac80211 should ignore the PM bit.
In addition, expose ieee80211_start_ps()/ieee80211_end_ps() to make
low-level drivers capable of controlling PS-mode.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 include/net/mac80211.h |   38 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/rx.c      |   19 ++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 62c0ce2..26a8cfb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1069,6 +1069,13 @@ enum ieee80211_tkip_key_type {
  *	to decrypt group addressed frames, then IBSS RSN support is still
  *	possible but software crypto will be used. Advertise the wiphy flag
  *	only in that case.
+ *
+ * @IEEE80211_HW_AP_LINK_PS: When operating in AP mode the device
+ *	autonomously manages the PS status of connected stations. When
+ *	this flag is set mac80211 will not trigger PS mode for connected
+ *	stations based on the PM bit of incoming frames.
+ *	Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
+ *	the PS mode of connected stations.
  */
 enum ieee80211_hw_flags {
 	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
@@ -1093,6 +1100,7 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_CONNECTION_MONITOR			= 1<<19,
 	IEEE80211_HW_SUPPORTS_CQM_RSSI			= 1<<20,
 	IEEE80211_HW_SUPPORTS_PER_STA_GTK		= 1<<21,
+	IEEE80211_HW_AP_LINK_PS				= 1<<22,
 };
 
 /**
@@ -2113,6 +2121,36 @@ static inline void ieee80211_rx_ni(struct ieee80211_hw *hw,
 	local_bh_enable();
 }
 
+/**
+ * ieee80211_start_ps - start PS for connected sta
+ *
+ * When operating in AP mode, use this function to inform mac80211
+ * about a station entering PS mode.
+ * Note that by default mac80211 will automatically call the internal
+ * equivalent of this function according to the PM bit of incoming frames.
+ * Use the IEEE80211_HW_AP_LINK_PS flag to change this.
+ *
+ * This function may not be called in IRQ context or process context.
+ *
+ * @sta: currently connected sta
+ */
+void ieee80211_start_ps(struct ieee80211_sta *sta);
+
+/**
+ * ieee80211_end_ps - end PS for connection sta
+ *
+ * When operating in AP mode, use this function to inform mac80211
+ * about a station exiting PS mode.
+ * Note that by default mac80211 will automatically call the internal
+ * equivalent of this function according to the PM bit of incoming frames.
+ * Use the IEEE80211_HW_AP_LINK_PS flag to change this.
+ *
+ * This function may not be called in IRQ context or process context.
+ *
+ * @sta: currently connected sta
+ */
+void ieee80211_end_ps(struct ieee80211_sta *sta);
+
 /*
  * The TX headroom reserved by mac80211 for its own tx_status functions.
  * This is enough for the radiotap header.
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a6701ed..c892271 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1095,6 +1095,14 @@ static void ap_sta_ps_start(struct sta_info *sta)
 #endif /* CONFIG_MAC80211_VERBOSE_PS_DEBUG */
 }
 
+void ieee80211_start_ps(struct ieee80211_sta *sta)
+{
+	struct sta_info *sta_inf = container_of(sta, struct sta_info, sta);
+
+	ap_sta_ps_start(sta_inf);
+}
+EXPORT_SYMBOL(ieee80211_start_ps);
+
 static void ap_sta_ps_end(struct sta_info *sta)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
@@ -1117,6 +1125,14 @@ static void ap_sta_ps_end(struct sta_info *sta)
 	ieee80211_sta_ps_deliver_wakeup(sta);
 }
 
+void ieee80211_end_ps(struct ieee80211_sta *sta)
+{
+	struct sta_info *sta_inf = container_of(sta, struct sta_info, sta);
+
+	ap_sta_ps_end(sta_inf);
+}
+EXPORT_SYMBOL(ieee80211_end_ps);
+
 static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 {
@@ -1161,7 +1177,8 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 	 * Change STA power saving mode only at the end of a frame
 	 * exchange sequence.
 	 */
-	if (!ieee80211_has_morefrags(hdr->frame_control) &&
+	if (!(sta->local->hw.flags & IEEE80211_HW_AP_LINK_PS) &&
+	    !ieee80211_has_morefrags(hdr->frame_control) &&
 	    !(status->rx_flags & IEEE80211_RX_DEFERRED_RELEASE) &&
 	    (rx->sdata->vif.type == NL80211_IFTYPE_AP ||
 	     rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN)) {
-- 
1.7.1

[PATCH 07/10] wl12xx: AP-mode - support HW based link PS monitoring

[Arik Nemtsov]

When operating in AP mode the wl1271 hardware filters our null-data
packets as well as management packets. This makes it impossible for
mac80211 to monitor the PS mode by using the PM bit of incoming frames.

Disable mac80211 automatic link PS-mode handling by supporting
IEEE80211_HW_AP_LINK_PS in HW flags.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 6318224..77c8974 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -3147,7 +3147,8 @@ int wl1271_init_ieee80211(struct wl1271 *wl)
 		IEEE80211_HW_SUPPORTS_UAPSD |
 		IEEE80211_HW_HAS_RATE_CONTROL |
 		IEEE80211_HW_CONNECTION_MONITOR |
-		IEEE80211_HW_SUPPORTS_CQM_RSSI;
+		IEEE80211_HW_SUPPORTS_CQM_RSSI |
+		IEEE80211_HW_AP_LINK_PS;
 
 	wl->hw->wiphy->cipher_suites = cipher_suites;
 	wl->hw->wiphy->n_cipher_suites = ARRAY_SIZE(cipher_suites);
-- 
1.7.1

[PATCH 08/10] wl12xx: AP mode - fix bug in cleanup of wl1271_op_sta_add()

[Arik Nemtsov]

Remove an active hlid when chip wakeup fails. In addition rename the
involved functions.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 77c8974..d600cff 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -2611,7 +2611,7 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
 	return 0;
 }
 
-static int wl1271_allocate_hlid(struct wl1271 *wl,
+static int wl1271_allocate_sta(struct wl1271 *wl,
 			     struct ieee80211_sta *sta,
 			     u8 *hlid)
 {
@@ -2632,10 +2632,13 @@ static int wl1271_allocate_hlid(struct wl1271 *wl,
 	return 0;
 }
 
-static void wl1271_free_hlid(struct wl1271 *wl, u8 hlid)
+static void wl1271_free_sta(struct wl1271 *wl, u8 hlid)
 {
 	int id = hlid - WL1271_AP_STA_HLID_START;
 
+	if (WARN_ON(!test_bit(id, wl->ap_hlid_map)))
+		return;
+
 	__clear_bit(id, wl->ap_hlid_map);
 	wl1271_tx_reset_link_queues(wl, hlid);
 }
@@ -2658,13 +2661,13 @@ static int wl1271_op_sta_add(struct ieee80211_hw *hw,
 
 	wl1271_debug(DEBUG_MAC80211, "mac80211 add sta %d", (int)sta->aid);
 
-	ret = wl1271_allocate_hlid(wl, sta, &hlid);
+	ret = wl1271_allocate_sta(wl, sta, &hlid);
 	if (ret < 0)
 		goto out;
 
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
-		goto out;
+		goto out_free_sta;
 
 	ret = wl1271_cmd_add_sta(wl, sta, hlid);
 	if (ret < 0)
@@ -2673,6 +2676,10 @@ static int wl1271_op_sta_add(struct ieee80211_hw *hw,
 out_sleep:
 	wl1271_ps_elp_sleep(wl);
 
+out_free_sta:
+	if (ret < 0)
+		wl1271_free_sta(wl, hlid);
+
 out:
 	mutex_unlock(&wl->mutex);
 	return ret;
@@ -2709,7 +2716,7 @@ static int wl1271_op_sta_remove(struct ieee80211_hw *hw,
 	if (ret < 0)
 		goto out_sleep;
 
-	wl1271_free_hlid(wl, wl_sta->hlid);
+	wl1271_free_sta(wl, wl_sta->hlid);
 
 out_sleep:
 	wl1271_ps_elp_sleep(wl);
-- 
1.7.1

[PATCH 09/10] wl12xx: AP-mode - count free FW TX blocks per link

[Arik Nemtsov]

Count the number of FW TX blocks allocated per link. We add blocks to a
link counter when allocated for a TX descriptor. We remove blocks
according to counters in fw_status indicating the number of freed blocks
in FW. These counters are polled after each IRQ.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c   |   11 +++++++
 drivers/net/wireless/wl12xx/ps.c     |    2 -
 drivers/net/wireless/wl12xx/tx.c     |   53 ++++++++++++++++++---------------
 drivers/net/wireless/wl12xx/wl12xx.h |    4 ++
 4 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index d600cff..a83d26d 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -549,6 +549,17 @@ static void wl1271_fw_status(struct wl1271 *wl,
 	if (total)
 		clear_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags);
 
+	if (wl->bss_type == BSS_TYPE_AP_BSS) {
+		for (i = 0; i < AP_MAX_LINKS; i++) {
+			u8 cnt = status->tx_lnk_free_blks[i] -
+				wl->links[i].prev_freed_blks;
+
+			wl->links[i].prev_freed_blks =
+				status->tx_lnk_free_blks[i];
+			wl->links[i].allocated_blks -= cnt;
+		}
+	}
+
 	/* update the host-chipset time offset */
 	getnstimeofday(&ts);
 	wl->time_offset = (timespec_to_ns(&ts) >> 10) -
diff --git a/drivers/net/wireless/wl12xx/ps.c b/drivers/net/wireless/wl12xx/ps.c
index 60a3738..92397ad 100644
--- a/drivers/net/wireless/wl12xx/ps.c
+++ b/drivers/net/wireless/wl12xx/ps.c
@@ -174,5 +174,3 @@ int wl1271_ps_set_mode(struct wl1271 *wl, enum wl1271_cmd_ps_mode mode,
 
 	return ret;
 }
-
-
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index d43193e..128c274 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -108,7 +108,7 @@ u8 wl1271_tx_get_hlid(struct sk_buff *skb)
 }
 
 static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
-				u32 buf_offset)
+				u32 buf_offset, u8 hlid)
 {
 	struct wl1271_tx_hw_descr *desc;
 	u32 total_len = skb->len + sizeof(struct wl1271_tx_hw_descr) + extra;
@@ -137,6 +137,9 @@ static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
 
 		wl->tx_blocks_available -= total_blocks;
 
+		if (wl->bss_type == BSS_TYPE_AP_BSS)
+			wl->links[hlid].allocated_blks += total_blocks;
+
 		ret = 0;
 
 		wl1271_debug(DEBUG_TX,
@@ -150,7 +153,8 @@ static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
 }
 
 static void wl1271_tx_fill_hdr(struct wl1271 *wl, struct sk_buff *skb,
-			      u32 extra, struct ieee80211_tx_info *control)
+			      u32 extra, struct ieee80211_tx_info *control,
+			      u8 hlid)
 {
 	struct timespec ts;
 	struct wl1271_tx_hw_descr *desc;
@@ -186,7 +190,7 @@ static void wl1271_tx_fill_hdr(struct wl1271 *wl, struct sk_buff *skb,
 	desc->tid = ac;
 
 	if (wl->bss_type != BSS_TYPE_AP_BSS) {
-		desc->aid = TX_HW_DEFAULT_AID;
+		desc->aid = hlid;
 
 		/* if the packets are destined for AP (have a STA entry)
 		   send them with AP rate policies, otherwise use default
@@ -196,25 +200,17 @@ static void wl1271_tx_fill_hdr(struct wl1271 *wl, struct sk_buff *skb,
 		else
 			rate_idx = ACX_TX_BASIC_RATE;
 	} else {
-		if (control->control.sta) {
-			struct wl1271_station *wl_sta;
-
-			wl_sta = (struct wl1271_station *)
-					control->control.sta->drv_priv;
-			desc->hlid = wl_sta->hlid;
+		desc->hlid = hlid;
+		switch (hlid) {
+		case WL1271_AP_GLOBAL_HLID:
+			rate_idx = ACX_TX_AP_MODE_MGMT_RATE;
+			break;
+		case WL1271_AP_BROADCAST_HLID:
+			rate_idx = ACX_TX_AP_MODE_BCST_RATE;
+			break;
+		default:
 			rate_idx = ac;
-		} else {
-			struct ieee80211_hdr *hdr;
-
-			hdr = (struct ieee80211_hdr *)
-						(skb->data + sizeof(*desc));
-			if (ieee80211_is_mgmt(hdr->frame_control)) {
-				desc->hlid = WL1271_AP_GLOBAL_HLID;
-				rate_idx = ACX_TX_AP_MODE_MGMT_RATE;
-			} else {
-				desc->hlid = WL1271_AP_BROADCAST_HLID;
-				rate_idx = ACX_TX_AP_MODE_BCST_RATE;
-			}
+			break;
 		}
 	}
 
@@ -245,6 +241,7 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
 	u32 extra = 0;
 	int ret = 0;
 	u32 total_len;
+	u8 hlid;
 
 	if (!skb)
 		return -EINVAL;
@@ -271,14 +268,19 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
 		}
 	}
 
-	ret = wl1271_tx_allocate(wl, skb, extra, buf_offset);
+	if (wl->bss_type == BSS_TYPE_AP_BSS)
+		hlid = wl1271_tx_get_hlid(skb);
+	else
+		hlid = TX_HW_DEFAULT_AID;
+
+	ret = wl1271_tx_allocate(wl, skb, extra, buf_offset, hlid);
 	if (ret < 0)
 		return ret;
 
 	if (wl->bss_type == BSS_TYPE_AP_BSS)
 		wl1271_tx_ap_update_inconnection_sta(wl, skb);
 
-	wl1271_tx_fill_hdr(wl, skb, extra, info);
+	wl1271_tx_fill_hdr(wl, skb, extra, info, hlid);
 
 	/*
 	 * The length of each packet is stored in terms of words. Thus, we must
@@ -656,8 +658,11 @@ void wl1271_tx_reset(struct wl1271 *wl)
 
 	/* TX failure */
 	if (wl->bss_type == BSS_TYPE_AP_BSS) {
-		for (i = 0; i < AP_MAX_LINKS; i++)
+		for (i = 0; i < AP_MAX_LINKS; i++) {
 			wl1271_tx_reset_link_queues(wl, i);
+			wl->links[i].allocated_blks = 0;
+			wl->links[i].prev_freed_blks = 0;
+		}
 
 		wl->last_tx_hlid = 0;
 	} else {
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index 0027e00..59d687b 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -271,6 +271,10 @@ struct wl1271_ap_key {
 struct wl1271_link {
 	/* AP-mode - TX queue per AC in link */
 	struct sk_buff_head tx_queue[NUM_TX_QUEUES];
+
+	/* accounting for allocated / available TX blocks in FW */
+	u8 allocated_blks;
+	u8 prev_freed_blks;
 };
 
 struct wl1271 {
-- 
1.7.1

[PATCH 10/10] wl12xx: AP-mode - management of links in PS-mode

[Arik Nemtsov]

Update the PS mode of each link according to a bitmap polled from
fw_status. Manually notify mac80211 about PS mode changes in connected
stations.

mac80211 will only be notified about PS start when the station is in PS
and there is a small number of TX blocks from this link ready in HW.
This is required for waking up the remote station since the TIM is
updated entirely by FW.

When a station enters mac80211-PS-mode, we drop all the skbs in the
low-level TX queues belonging to this sta with STAT_TX_FILTERED
to keep our queues clean.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c   |   73 ++++++++++++++++++++++++++-----
 drivers/net/wireless/wl12xx/ps.c     |   80 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/ps.h     |    2 +
 drivers/net/wireless/wl12xx/tx.c     |   24 ++++++++++-
 drivers/net/wireless/wl12xx/wl12xx.h |   19 ++++++++
 5 files changed, 186 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index a83d26d..d362a30 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -518,6 +518,57 @@ static int wl1271_plt_init(struct wl1271 *wl)
 	return ret;
 }
 
+static void wl1271_irq_ps_regulate_link(struct wl1271 *wl, u8 hlid, u8 tx_blks)
+{
+	bool fw_ps;
+
+	/* only regulate station links */
+	if (hlid < WL1271_AP_STA_HLID_START)
+		return;
+
+	fw_ps = test_bit(hlid, (unsigned long *)&wl->ap_fw_ps_map);
+
+	/*
+	 * Wake up from high level PS if the STA is asleep with too little
+	 * blocks in FW or if the STA is awake.
+	 */
+	if (!fw_ps || tx_blks < WL1271_PS_STA_MAX_BLOCKS)
+		wl1271_ps_link_end(wl, hlid);
+
+	/* Start high-level PS if the STA is asleep with enough blocks in FW */
+	else if (fw_ps && tx_blks >= WL1271_PS_STA_MAX_BLOCKS)
+		wl1271_ps_link_start(wl, hlid, true);
+}
+
+static void wl1271_irq_update_links_status(struct wl1271 *wl,
+				       struct wl1271_fw_status *status)
+{
+	u32 cur_fw_ps_map;
+	u8 hlid;
+
+	cur_fw_ps_map = le32_to_cpu(status->link_ps_bitmap);
+	if (wl->ap_fw_ps_map != cur_fw_ps_map) {
+		wl1271_debug(DEBUG_PSM,
+			     "link ps prev 0x%x cur 0x%x changed 0x%x",
+			     wl->ap_fw_ps_map, cur_fw_ps_map,
+			     wl->ap_fw_ps_map ^ cur_fw_ps_map);
+
+		wl->ap_fw_ps_map = cur_fw_ps_map;
+	}
+
+	for (hlid = WL1271_AP_STA_HLID_START; hlid < AP_MAX_LINKS; hlid++) {
+		u8 cnt = status->tx_lnk_free_blks[hlid] -
+			wl->links[hlid].prev_freed_blks;
+
+		wl->links[hlid].prev_freed_blks =
+			status->tx_lnk_free_blks[hlid];
+		wl->links[hlid].allocated_blks -= cnt;
+
+		wl1271_irq_ps_regulate_link(wl, hlid,
+					    wl->links[hlid].allocated_blks);
+	}
+}
+
 static void wl1271_fw_status(struct wl1271 *wl,
 			     struct wl1271_fw_status *status)
 {
@@ -549,16 +600,9 @@ static void wl1271_fw_status(struct wl1271 *wl,
 	if (total)
 		clear_bit(WL1271_FLAG_FW_TX_BUSY, &wl->flags);
 
-	if (wl->bss_type == BSS_TYPE_AP_BSS) {
-		for (i = 0; i < AP_MAX_LINKS; i++) {
-			u8 cnt = status->tx_lnk_free_blks[i] -
-				wl->links[i].prev_freed_blks;
-
-			wl->links[i].prev_freed_blks =
-				status->tx_lnk_free_blks[i];
-			wl->links[i].allocated_blks -= cnt;
-		}
-	}
+	/* for AP update num of allocated TX blocks per link and ps status */
+	if (wl->bss_type == BSS_TYPE_AP_BSS)
+		wl1271_irq_update_links_status(wl, status);
 
 	/* update the host-chipset time offset */
 	getnstimeofday(&ts);
@@ -1239,6 +1283,8 @@ static void __wl1271_op_remove_interface(struct wl1271 *wl)
 	wl->filters = 0;
 	wl1271_free_ap_keys(wl);
 	memset(wl->ap_hlid_map, 0, sizeof(wl->ap_hlid_map));
+	wl->ap_fw_ps_map = 0;
+	wl->ap_ps_map = 0;
 
 	for (i = 0; i < NUM_TX_QUEUES; i++)
 		wl->tx_blocks_freed[i] = 0;
@@ -2636,10 +2682,10 @@ static int wl1271_allocate_sta(struct wl1271 *wl,
 	}
 
 	wl_sta = (struct wl1271_station *)sta->drv_priv;
-
 	__set_bit(id, wl->ap_hlid_map);
 	wl_sta->hlid = WL1271_AP_STA_HLID_START + id;
 	*hlid = wl_sta->hlid;
+	memcpy(wl->links[wl_sta->hlid].addr, sta->addr, ETH_ALEN);
 	return 0;
 }
 
@@ -2651,7 +2697,10 @@ static void wl1271_free_sta(struct wl1271 *wl, u8 hlid)
 		return;
 
 	__clear_bit(id, wl->ap_hlid_map);
+	memset(wl->links[hlid].addr, 0, ETH_ALEN);
 	wl1271_tx_reset_link_queues(wl, hlid);
+	__clear_bit(hlid, &wl->ap_ps_map);
+	__clear_bit(hlid, (unsigned long *)&wl->ap_fw_ps_map);
 }
 
 static int wl1271_op_sta_add(struct ieee80211_hw *hw,
@@ -3275,6 +3324,8 @@ struct ieee80211_hw *wl1271_alloc_hw(void)
 	wl->set_bss_type = MAX_BSS_TYPE;
 	wl->fw_bss_type = MAX_BSS_TYPE;
 	wl->last_tx_hlid = 0;
+	wl->ap_ps_map = 0;
+	wl->ap_fw_ps_map = 0;
 
 	memset(wl->tx_frames_map, 0, sizeof(wl->tx_frames_map));
 	for (i = 0; i < ACX_TX_DESCRIPTORS; i++)
diff --git a/drivers/net/wireless/wl12xx/ps.c b/drivers/net/wireless/wl12xx/ps.c
index 92397ad..eb80192 100644
--- a/drivers/net/wireless/wl12xx/ps.c
+++ b/drivers/net/wireless/wl12xx/ps.c
@@ -24,6 +24,7 @@
 #include "reg.h"
 #include "ps.h"
 #include "io.h"
+#include "tx.h"
 
 #define WL1271_WAKEUP_TIMEOUT 500
 
@@ -174,3 +175,82 @@ int wl1271_ps_set_mode(struct wl1271 *wl, enum wl1271_cmd_ps_mode mode,
 
 	return ret;
 }
+
+static void wl1271_ps_filter_frames(struct wl1271 *wl, u8 hlid)
+{
+	int i, filtered = 0;
+	struct sk_buff *skb;
+	struct ieee80211_tx_info *info;
+	unsigned long flags;
+
+	/* filter all frames currently the low level queus for this hlid */
+	for (i = 0; i < NUM_TX_QUEUES; i++) {
+		while ((skb = skb_dequeue(&wl->links[hlid].tx_queue[i]))) {
+			info = IEEE80211_SKB_CB(skb);
+			info->flags |= IEEE80211_TX_STAT_TX_FILTERED;
+			info->status.rates[0].idx = -1;
+			ieee80211_tx_status(wl->hw, skb);
+			filtered++;
+		}
+	}
+
+	spin_lock_irqsave(&wl->wl_lock, flags);
+	wl->tx_queue_count -= filtered;
+	spin_unlock_irqrestore(&wl->wl_lock, flags);
+
+	wl1271_handle_tx_low_watermark(wl);
+}
+
+void wl1271_ps_link_start(struct wl1271 *wl, u8 hlid, bool clean_queues)
+{
+	struct ieee80211_sta *sta;
+
+	if (test_bit(hlid, &wl->ap_ps_map))
+		return;
+
+	wl1271_debug(DEBUG_PSM, "start mac80211 PSM on hlid %d blks %d "
+		     "clean_queues %d", hlid, wl->links[hlid].allocated_blks,
+		     clean_queues);
+
+	rcu_read_lock();
+	sta = ieee80211_find_sta(wl->vif, wl->links[hlid].addr);
+	if (!sta) {
+		wl1271_error("could not find sta %pM for starting ps",
+			     wl->links[hlid].addr);
+		rcu_read_unlock();
+		return;
+	}
+
+	ieee80211_start_ps(sta);
+	rcu_read_unlock();
+
+	/* do we want to filter all frames from this link's queues? */
+	if (clean_queues)
+		wl1271_ps_filter_frames(wl, hlid);
+
+	__set_bit(hlid, &wl->ap_ps_map);
+}
+
+void wl1271_ps_link_end(struct wl1271 *wl, u8 hlid)
+{
+	struct ieee80211_sta *sta;
+
+	if (!test_bit(hlid, &wl->ap_ps_map))
+		return;
+
+	wl1271_debug(DEBUG_PSM, "end mac80211 PSM on hlid %d", hlid);
+
+	__clear_bit(hlid, &wl->ap_ps_map);
+
+	rcu_read_lock();
+	sta = ieee80211_find_sta(wl->vif, wl->links[hlid].addr);
+	if (!sta) {
+		wl1271_error("could not find sta %pM for ending ps",
+			     wl->links[hlid].addr);
+		goto end;
+	}
+
+	ieee80211_end_ps(sta);
+end:
+	rcu_read_unlock();
+}
diff --git a/drivers/net/wireless/wl12xx/ps.h b/drivers/net/wireless/wl12xx/ps.h
index 8415060..fc1f4c1 100644
--- a/drivers/net/wireless/wl12xx/ps.h
+++ b/drivers/net/wireless/wl12xx/ps.h
@@ -32,5 +32,7 @@ int wl1271_ps_set_mode(struct wl1271 *wl, enum wl1271_cmd_ps_mode mode,
 void wl1271_ps_elp_sleep(struct wl1271 *wl);
 int wl1271_ps_elp_wakeup(struct wl1271 *wl, bool chip_awake);
 void wl1271_elp_work(struct work_struct *work);
+void wl1271_ps_link_start(struct wl1271 *wl, u8 hlid, bool clean_queues);
+void wl1271_ps_link_end(struct wl1271 *wl, u8 hlid);
 
 #endif /* __WL1271_PS_H__ */
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index 128c274..e515b72 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -86,6 +86,26 @@ static void wl1271_tx_ap_update_inconnection_sta(struct wl1271 *wl,
 		wl1271_acx_set_inconnection_sta(wl, hdr->addr1);
 }
 
+static void wl1271_tx_regulate_link(struct wl1271 *wl, u8 hlid)
+{
+	bool fw_ps;
+	u8 tx_blks;
+
+	/* only regulate station links */
+	if (hlid < WL1271_AP_STA_HLID_START)
+		return;
+
+	fw_ps = test_bit(hlid, (unsigned long *)&wl->ap_fw_ps_map);
+	tx_blks = wl->links[hlid].allocated_blks;
+
+	/*
+	 * if in FW PS and there is enough data in FW we can put the link
+	 * into high-level PS and clean out its TX queues.
+	 */
+	if (fw_ps && tx_blks >= WL1271_PS_STA_MAX_BLOCKS)
+		wl1271_ps_link_start(wl, hlid, true);
+}
+
 u8 wl1271_tx_get_hlid(struct sk_buff *skb)
 {
 	struct ieee80211_tx_info *control = IEEE80211_SKB_CB(skb);
@@ -277,8 +297,10 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
 	if (ret < 0)
 		return ret;
 
-	if (wl->bss_type == BSS_TYPE_AP_BSS)
+	if (wl->bss_type == BSS_TYPE_AP_BSS) {
 		wl1271_tx_ap_update_inconnection_sta(wl, skb);
+		wl1271_tx_regulate_link(wl, hlid);
+	}
 
 	wl1271_tx_fill_hdr(wl, skb, extra, info, hlid);
 
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index 59d687b..7cb3d5b 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -146,6 +146,17 @@ extern u32 wl12xx_debug_level;
 #define WL1271_AP_BROADCAST_HLID   1
 #define WL1271_AP_STA_HLID_START   2
 
+/*
+ * When in AP-mode, we allow (at least) this number of mem-blocks
+ * to be transmitted to FW for a STA in PS-mode. Only when packets are
+ * present in the FW buffers it will wake the sleeping STA. We want to put
+ * enough packets for the driver to transmit all of its buffered data before
+ * the STA goes to sleep again. But we don't want to take too much mem-blocks
+ * as it might hurt the throughput of active STAs.
+ * The number of blocks (18) is enough for 2 large packets.
+ */
+#define WL1271_PS_STA_MAX_BLOCKS  (2 * 9)
+
 #define WL1271_AP_BSS_INDEX        0
 #define WL1271_AP_DEF_INACTIV_SEC  300
 #define WL1271_AP_DEF_BEACON_EXP   20
@@ -275,6 +286,8 @@ struct wl1271_link {
 	/* accounting for allocated / available TX blocks in FW */
 	u8 allocated_blks;
 	u8 prev_freed_blks;
+
+	u8 addr[ETH_ALEN];
 };
 
 struct wl1271 {
@@ -478,6 +491,12 @@ struct wl1271 {
 
 	/* the hlid of the link where the last transmitted skb came from */
 	int last_tx_hlid;
+
+	/* AP-mode - a bitmap of links currently in PS mode according to FW */
+	u32 ap_fw_ps_map;
+
+	/* AP-mode - a bitmap of links currently in PS mode in mac80211 */
+	unsigned long ap_ps_map;
 };
 
 struct wl1271_station {
-- 
1.7.1

Re: [PATCH 04/10] mac80211: do not calc frame duration when using HW rate-control

[Johannes Berg]

On Sun, 2011-01-16 at 07:42 +0200, Arik Nemtsov wrote:
> When rate-control is performed in HW, we cannot calculate frame
> duration as we do not have the skb transmission rate in SW.
> 
> ieee80211_tx_h_calculate_duration() should only be called when
> ieee80211_tx_h_rate_ctrl() has been called before to initialize data
> in skb->cb. This doesn't happen for drivers with HW rate-control.
> 
> Fixes the following warning when operating in AP-mode
> in a driver with HW rate-control.
> 
> <4>[16747.586029] WARNING: at net/mac80211/tx.c:57 ieee80211_duration
> +0x54/0x1d8 [mac80211]()
> <4>[16747.594085] Modules linked in: wl12xx_sdio wl12xx firmware_class crc7
> mac80211 cfg80211 [last unloaded: crc7]
> <4>[16747.604095] [<c004707c>] (unwind_backtrace+0x0/0x124) from [<c006644c>]
> (warn_slowpath_common+0x4c/0x64)
> <4>[16747.613616] [<c006644c>] (warn_slowpath_common+0x4c/0x64) from
> [<c006647c>] (warn_slowpath_null+0x18/0x1c)
> <4>[16747.623382] [<c006647c>] (warn_slowpath_null+0x18/0x1c) from [<bf1aa8e8>]
> (ieee80211_duration+0x54/0x1d8 [mac80211])
> <4>[16747.634124] [<bf1aa8e8>] (ieee80211_duration+0x54/0x1d8 [mac80211]) from
> [<bf1aacec>] (ieee80211_tx_h_calculate_duration+0x3c/0x5c [mac80211])
> <4>[16747.647094] [<bf1aacec>] (ieee80211_tx_h_calculate_duration+0x3c/0x5c
> [mac80211]) from [<bf1ab21c>] (invoke_tx_handlers+0xdc/0x118 [mac80211])
> <4>[16747.660064] [<bf1ab21c>] (invoke_tx_handlers+0xdc/0x118 [mac80211])
> from [<bf1ab2dc>] (ieee80211_tx+0x84/0x248 [mac80211])
> <4>[16747.671295] [<bf1ab2dc>] (ieee80211_tx+0x84/0x248 [mac80211]) from
> [<bf1abe3c>] (ieee80211_tx_pending+0x12c/0x278 [mac80211])
> <4>[16747.682739] [<bf1abe3c>] (ieee80211_tx_pending+0x12c/0x278 [mac80211])
> from [<c006b3bc>] (tasklet_action+0x68/0xc0)
> 
> Signed-off-by: Arik Nemtsov <arik@wizery.com>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

though I wish the backtrace in your commit log wasn't all mangled and
word-wrapped.

johannes

> ---
>  net/mac80211/tx.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 5950e3a..e0d6d5c 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1394,7 +1394,8 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
>  	/* handlers after fragment must be aware of tx info fragmentation! */
>  	CALL_TXH(ieee80211_tx_h_stats);
>  	CALL_TXH(ieee80211_tx_h_encrypt);
> -	CALL_TXH(ieee80211_tx_h_calculate_duration);
> +	if (!(tx->local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL))
> +		CALL_TXH(ieee80211_tx_h_calculate_duration);
>  #undef CALL_TXH
>  
>   txh_done:

Re: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Johannes Berg]

On Sun, 2011-01-16 at 07:42 +0200, Arik Nemtsov wrote:
> When operating in AP mode the wl1271 hardware filters our null-data
> packets as well as management packets. This makes it impossible for
> mac80211 to monitor the PS mode by using the PM bit of incoming frames.
> 
> Implement a HW flag to indicate that mac80211 should ignore the PM bit.
> In addition, expose ieee80211_start_ps()/ieee80211_end_ps() to make
> low-level drivers capable of controlling PS-mode.
> 
> Signed-off-by: Arik Nemtsov <arik@wizery.com>
> ---
>  include/net/mac80211.h |   38 ++++++++++++++++++++++++++++++++++++++
>  net/mac80211/rx.c      |   19 ++++++++++++++++++-
>  2 files changed, 56 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 62c0ce2..26a8cfb 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1069,6 +1069,13 @@ enum ieee80211_tkip_key_type {
>   *	to decrypt group addressed frames, then IBSS RSN support is still
>   *	possible but software crypto will be used. Advertise the wiphy flag
>   *	only in that case.
> + *
> + * @IEEE80211_HW_AP_LINK_PS: When operating in AP mode the device
> + *	autonomously manages the PS status of connected stations. When
> + *	this flag is set mac80211 will not trigger PS mode for connected
> + *	stations based on the PM bit of incoming frames.
> + *	Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
> + *	the PS mode of connected stations.
>   */
>  enum ieee80211_hw_flags {
>  	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
> @@ -1093,6 +1100,7 @@ enum ieee80211_hw_flags {
>  	IEEE80211_HW_CONNECTION_MONITOR			= 1<<19,
>  	IEEE80211_HW_SUPPORTS_CQM_RSSI			= 1<<20,
>  	IEEE80211_HW_SUPPORTS_PER_STA_GTK		= 1<<21,
> +	IEEE80211_HW_AP_LINK_PS				= 1<<22,
>  };
>  
>  /**
> @@ -2113,6 +2121,36 @@ static inline void ieee80211_rx_ni(struct ieee80211_hw *hw,
>  	local_bh_enable();
>  }
>  
> +/**
> + * ieee80211_start_ps - start PS for connected sta
> + *
> + * When operating in AP mode, use this function to inform mac80211
> + * about a station entering PS mode.
> + * Note that by default mac80211 will automatically call the internal
> + * equivalent of this function according to the PM bit of incoming frames.
> + * Use the IEEE80211_HW_AP_LINK_PS flag to change this.

I think that explanation should be clearer and say that you must call
this only when the flag is set and vice versa. Also, there should be a 

WARN_ON(hw->flags & HW_AP_LINK_PS)

in the functions.

Also, maybe just a single function with a bool argument would be more
appropriate? Call it "ieee80211_sta_ps_transition()" or something like
that. You should also remove the text about calling the internal
equivalent -- the internal details might change at any time but the API
should be documented in a way that makes sense without the internal
details for the benefit of both sides -- people implementing a driver
don't need to know about the internal details, and people changing the
internal details know what semantics to keep.

> + * This function may not be called in IRQ context or process context.

This can't be true -- the latter part you must mean "or with softirqs
enabled".


Also, I'm worried about races between this and RX. All of the RX path
assumes that it is running at the same time. The ap_sta_ps_{start,end}
functions won't be called from the RX path when the flag is set, and
this is dependent on setting the flag, but is there really nothing in
them that could race?

Finally, I'm worried about this calling back into the driver's
sta_notify callback. If that is to remain that way, it needs to be
*very* clearly documented, I'd *much* prefer it not doing that but
handing off to a work struct or so internally.

johannes

Fwd: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Arik Nemtsov]

[ resending - html mail rejected ]

On Sun, Jan 16, 2011 at 10:50, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > +/**
> > + * ieee80211_start_ps - start PS for connected sta
> > + *
> > + * When operating in AP mode, use this function to inform mac80211
> > + * about a station entering PS mode.
> > + * Note that by default mac80211 will automatically call the internal
> > + * equivalent of this function according to the PM bit of incoming frames.
> > + * Use the IEEE80211_HW_AP_LINK_PS flag to change this.
>
> I think that explanation should be clearer and say that you must call
> this only when the flag is set and vice versa. Also, there should be a
>
> WARN_ON(hw->flags & HW_AP_LINK_PS)
>
> in the functions.

will fix in v2.

>
> Also, maybe just a single function with a bool argument would be more
> appropriate? Call it "ieee80211_sta_ps_transition()" or something like
> that. You should also remove the text about calling the internal
> equivalent -- the internal details might change at any time but the API
> should be documented in a way that makes sense without the internal
> details for the benefit of both sides -- people implementing a driver
> don't need to know about the internal details, and people changing the
> internal details know what semantics to keep.

a single function sounds good. v2.

>
> > + * This function may not be called in IRQ context or process context.
>
> This can't be true -- the latter part you must mean "or with softirqs
> enabled".

Yes you're right.
(While looking at this I discovered that I forgot to disable softirqs
myself. These functions are called from a workqueue in wl12xx - will
be fixed in v2)

>
> Also, I'm worried about races between this and RX. All of the RX path
> assumes that it is running at the same time. The ap_sta_ps_{start,end}
> functions won't be called from the RX path when the flag is set, and
> this is dependent on setting the flag, but is there really nothing in
> them that could race?

They might race if the user is not careful. On a SMP machine a user
can call ieee80211_rx() and ap_sta_ps_start() from two different
workqueues for example.
I can add an explicit mutex, but a documentation warning will suffice here no?

>
> Finally, I'm worried about this calling back into the driver's
> sta_notify callback. If that is to remain that way, it needs to be
> *very* clearly documented, I'd *much* prefer it not doing that but
> handing off to a work struct or so internally.
>

sta_notify is pretty useless when the low level driver calls toggles
the PS-mode by itself. How about I simply remove the call to
sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
If a low level driver needs to do some deferred processing after a
PS-mode transition, it can queue a work on its own..

Regards,
Arik

Re: Fwd: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Johannes Berg]

On Sun, 2011-01-16 at 23:53 +0200, Arik Nemtsov wrote:

> > Also, I'm worried about races between this and RX. All of the RX path
> > assumes that it is running at the same time. The ap_sta_ps_{start,end}
> > functions won't be called from the RX path when the flag is set, and
> > this is dependent on setting the flag, but is there really nothing in
> > them that could race?
> 
> They might race if the user is not careful. On a SMP machine a user
> can call ieee80211_rx() and ap_sta_ps_start() from two different
> workqueues for example.
> I can add an explicit mutex, but a documentation warning will suffice here no?

Well, a mutex won't be possible, and we don't do "code path locking"
anyway. I'd appreciate if you could actually see which things would
potentially race -- I took a brief look into these functions and it
didn't look like there are actually races except of these two against
each other maybe?

> > Finally, I'm worried about this calling back into the driver's
> > sta_notify callback. If that is to remain that way, it needs to be
> > *very* clearly documented, I'd *much* prefer it not doing that but
> > handing off to a work struct or so internally.
> >
> 
> sta_notify is pretty useless when the low level driver calls toggles
> the PS-mode by itself. How about I simply remove the call to
> sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> If a low level driver needs to do some deferred processing after a
> PS-mode transition, it can queue a work on its own..

Yes, that's probably a good idea -- but definitely needs to be
documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
in a way. Maybe that needs to be done for the function call as well,
maybe via a return value?

johannes

Re: Fwd: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Arik Nemtsov]

On Mon, Jan 17, 2011 at 11:35, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Sun, 2011-01-16 at 23:53 +0200, Arik Nemtsov wrote:
>
> > > Also, I'm worried about races between this and RX. All of the RX path
> > > assumes that it is running at the same time. The ap_sta_ps_{start,end}
> > > functions won't be called from the RX path when the flag is set, and
> > > this is dependent on setting the flag, but is there really nothing in
> > > them that could race?
> >
> > They might race if the user is not careful. On a SMP machine a user
> > can call ieee80211_rx() and ap_sta_ps_start() from two different
> > workqueues for example.
> > I can add an explicit mutex, but a documentation warning will suffice here no?
>
> Well, a mutex won't be possible, and we don't do "code path locking"

I meant a spinlock of course (but its not really a good idea)

>
> anyway. I'd appreciate if you could actually see which things would
> potentially race -- I took a brief look into these functions and it
> didn't look like there are actually races except of these two against
> each other maybe?

Upon closer examination, I could find no races in the RX path. A race
of ps_start/stop is possible of course, but I guess we can demand the
calls to be synchronized against each other?

I did notice some concurrency that can happen in the TX path. I think
the same essential race is described in
ieee80211_handle_filtered_frame() between RX and TX paths. Only this
time the PS state is changed manually and not in the RX handler. The
warning in ieee80211_handle_filtered_frame() can be expanded to
include:
"always change PS state of connected stations before TX status events
if ordering can be unknown, for example with different interrupt
status bits."

> > sta_notify is pretty useless when the low level driver calls toggles
> > the PS-mode by itself. How about I simply remove the call to
> > sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> > If a low level driver needs to do some deferred processing after a
> > PS-mode transition, it can queue a work on its own..
>
> Yes, that's probably a good idea -- but definitely needs to be
> documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
> in a way. Maybe that needs to be done for the function call as well,
> maybe via a return value?
>

I'm afraid I didn't catch your meaning here.

Regards,
Arik

Re: Fwd: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Johannes Berg]

On Tue, 2011-01-18 at 00:47 +0200, Arik Nemtsov wrote:

> > anyway. I'd appreciate if you could actually see which things would
> > potentially race -- I took a brief look into these functions and it
> > didn't look like there are actually races except of these two against
> > each other maybe?
> 
> Upon closer examination, I could find no races in the RX path. A race
> of ps_start/stop is possible of course, but I guess we can demand the
> calls to be synchronized against each other?

Yeah that seems fair. It'd be a strange driver too anyway, come to think
of it, since this is all from the device so needs to be processed from a
tasklet or whatever anyway.

> I did notice some concurrency that can happen in the TX path. I think
> the same essential race is described in
> ieee80211_handle_filtered_frame() between RX and TX paths. Only this
> time the PS state is changed manually and not in the RX handler. The
> warning in ieee80211_handle_filtered_frame() can be expanded to
> include:
> "always change PS state of connected stations before TX status events
> if ordering can be unknown, for example with different interrupt
> status bits."

Right, that was RX first, then TX status, I guess it can be extended a
bit. But really, this race doesn't apply since the entire PS state
management is done by the device here, and mac80211 just follows it.

> > > sta_notify is pretty useless when the low level driver calls toggles
> > > the PS-mode by itself. How about I simply remove the call to
> > > sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> > > If a low level driver needs to do some deferred processing after a
> > > PS-mode transition, it can queue a work on its own..
> >
> > Yes, that's probably a good idea -- but definitely needs to be
> > documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
> > in a way. Maybe that needs to be done for the function call as well,
> > maybe via a return value?
> >
> 
> I'm afraid I didn't catch your meaning here.

Well if for instance the station happens to send two frames with the PM
bit set, meaning that it is in PS-poll mode but doesn't want to wake up
even though it's transmitting a frame, mac80211 will only call
sta_notify once for the first frame to tell the driver the station went
to sleep -- the second frame just indicates that it's still asleep.

The same might happen with this, but of course it depends on what the
device does. If the driver will call the new API function twice, then
mac80211 will be very confused. Therefore, I think the new API function
should do some checks about whether the station is already asleep/awake
before invoking ap_sta_ps_start/end so that the counters are correct.

In this case, the driver might want to know if the station was already
in the indicated state -- this might be indicated by the return value.

johannes

Re: Fwd: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Arik Nemtsov]

On Tue, Jan 18, 2011 at 11:05, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2011-01-18 at 00:47 +0200, Arik Nemtsov wrote:
>
>
> Well if for instance the station happens to send two frames with the PM
> bit set, meaning that it is in PS-poll mode but doesn't want to wake up
> even though it's transmitting a frame, mac80211 will only call
> sta_notify once for the first frame to tell the driver the station went
> to sleep -- the second frame just indicates that it's still asleep.
>
> The same might happen with this, but of course it depends on what the
> device does. If the driver will call the new API function twice, then
> mac80211 will be very confused. Therefore, I think the new API function
> should do some checks about whether the station is already asleep/awake
> before invoking ap_sta_ps_start/end so that the counters are correct.
>
> In this case, the driver might want to know if the station was already
> in the indicated state -- this might be indicated by the return value.
>

Yea i missed the double atomic_inc/dec in case the function is called
twice. I'll add a EINVAL return value in case the requested PM state
is already in effect.

Thanks,
Arik

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Luciano Coelho]

On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
> If a sta starts transmitting immediately after authentication, sometimes
> the FW deauthenticates it. Fix this by marking the sta "in-connection"
> in FW before sending the autentication response.
> 
> Signed-off-by: Arik Nemtsov <arik@wizery.com>
> ---

Looks good.  So you don't need to remove the STA from the inconnection
list later? Does the firmware remove it automatically?


> diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
> index fdec4a3..d3ba27c 100644
> --- a/drivers/net/wireless/wl12xx/tx.c
> +++ b/drivers/net/wireless/wl12xx/tx.c
> @@ -70,6 +70,22 @@ static void wl1271_free_tx_id(struct wl1271 *wl, int id)
>  	}
>  }
>  
> +static void wl1271_tx_ap_update_inconnection_sta(struct wl1271 *wl,
> +					      struct sk_buff *skb)

This indentation seems wrong.


> +	/*
> +	 * add the station to the known list before broadcasting the
> +	 * authentication response. this way it won't get de-authed by FW
> +	 * when transmitting too soon.
> +	 */

Broadcasting the auth response? Isn't it unicast?


> @@ -238,6 +254,9 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (wl->bss_type == BSS_TYPE_AP_BSS)
> +		wl1271_tx_ap_update_inconnection_sta(wl, skb);
> +
>  	wl1271_tx_fill_hdr(wl, skb, extra, info);

Isn't there any other place where you can put this? Doesn't it add too
much overhead if you do it for every TX frame?


-- 
Cheers,
Luca.

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Arik Nemtsov]

On Wed, Jan 26, 2011 at 16:01, Luciano Coelho <coelho@ti.com> wrote:
> On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
>> If a sta starts transmitting immediately after authentication, sometimes
>> the FW deauthenticates it. Fix this by marking the sta "in-connection"
>> in FW before sending the autentication response.
>>
>> Signed-off-by: Arik Nemtsov <arik@wizery.com>
>> ---
>
> Looks good.  So you don't need to remove the STA from the inconnection
> list later? Does the firmware remove it automatically?

Yes. There's a timeout period after which the STA is removed.

>
>
>> diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
>> index fdec4a3..d3ba27c 100644
>> --- a/drivers/net/wireless/wl12xx/tx.c
>> +++ b/drivers/net/wireless/wl12xx/tx.c
>> @@ -70,6 +70,22 @@ static void wl1271_free_tx_id(struct wl1271 *wl, int id)
>>       }
>>  }
>>
>> +static void wl1271_tx_ap_update_inconnection_sta(struct wl1271 *wl,
>> +                                           struct sk_buff *skb)
>
> This indentation seems wrong.

will fix.

>
>
>> +     /*
>> +      * add the station to the known list before broadcasting the
>> +      * authentication response. this way it won't get de-authed by FW
>> +      * when transmitting too soon.
>> +      */
>
> Broadcasting the auth response? Isn't it unicast?

I meant "transmitting". Thanks.

>
>
>> @@ -238,6 +254,9 @@ static int wl1271_prepare_tx_frame(struct wl1271 *wl, struct sk_buff *skb,
>>       if (ret < 0)
>>               return ret;
>>
>> +     if (wl->bss_type == BSS_TYPE_AP_BSS)
>> +             wl1271_tx_ap_update_inconnection_sta(wl, skb);
>> +
>>       wl1271_tx_fill_hdr(wl, skb, extra, info);
>
> Isn't there any other place where you can put this? Doesn't it add too
> much overhead if you do it for every TX frame?

At this stage the STA has not been officially added yet, so this is
just a packet. In fact it won't be added unless authenticated. When
the STA is added its too late. If we failed to mark it in-connection,
sometimes the FW will disconnect it.

Performance wise - its just another "compare + jmp" statement
basically (checking the packet header). Didn't see any effect on
throughput.

Regards,
Arik

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Luciano Coelho]

On Wed, 2011-01-26 at 21:04 +0100, Arik Nemtsov wrote:
> >> @@ -238,6 +254,9 @@ static int wl1271_prepare_tx_frame(struct
> wl1271 *wl, struct sk_buff *skb,
> >>       if (ret < 0)
> >>               return ret;
> >>
> >> +     if (wl->bss_type == BSS_TYPE_AP_BSS)
> >> +             wl1271_tx_ap_update_inconnection_sta(wl, skb);
> >> +
> >>       wl1271_tx_fill_hdr(wl, skb, extra, info);
> >
> > Isn't there any other place where you can put this? Doesn't it add
> too
> > much overhead if you do it for every TX frame?
> 
> At this stage the STA has not been officially added yet, so this is
> just a packet. In fact it won't be added unless authenticated. When
> the STA is added its too late. If we failed to mark it in-connection,
> sometimes the FW will disconnect it.
> 
> Performance wise - its just another "compare + jmp" statement
> basically (checking the packet header). Didn't see any effect on
> throughput. 

No, it's not just a cmp + jmp.  You actually call
wl1271_tx_ap_update_inconnection_sta() and there you dig out the header
and call ieee80211_is_auth(), so you're actually parsing the header of
every frame you transmit if you're an AP.  Isn't there a way to avoid
this only when necessary?

-- 
Cheers,
Luca.

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Johannes Berg]

On Sun, 2011-01-16 at 07:42 +0200, Arik Nemtsov wrote:
> If a sta starts transmitting immediately after authentication, sometimes
> the FW deauthenticates it. Fix this by marking the sta "in-connection"
> in FW before sending the autentication response.

Starts transmitting what? If it starts transmitting data frames, it's
supposed to be deauthenticated ... it mustn't do that before it has
associated?!

johannes

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Arik Nemtsov]

On Wed, Jan 26, 2011 at 23:33, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2011-01-16 at 07:42 +0200, Arik Nemtsov wrote:
>> If a sta starts transmitting immediately after authentication, sometimes
>> the FW deauthenticates it. Fix this by marking the sta "in-connection"
>> in FW before sending the autentication response.
>
> Starts transmitting what? If it starts transmitting data frames, it's
> supposed to be deauthenticated ... it mustn't do that before it has
> associated?!
>

Well no it doesn't do that. But the FW takes some time to know its
authenticated.
The flow is this:

1. hostapd sends auth complete (the FW doesn't know anything about this)
2. The STA transmits something
3. The AP FW deauths the STA
4. hostapd adds the station, causing mac80211 to call add_sta(), which
causes the FW to add the sta

Then we have an endless loop of sta add/remove..

Regards,
Arik

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Johannes Berg]

On Wed, 2011-01-26 at 23:44 +0200, Arik Nemtsov wrote:

> Well no it doesn't do that. But the FW takes some time to know its
> authenticated.
> The flow is this:
> 
> 1. hostapd sends auth complete (the FW doesn't know anything about this)

you mean assoc, right?

> 2. The STA transmits something
> 3. The AP FW deauths the STA
> 4. hostapd adds the station, causing mac80211 to call add_sta(), which
> causes the FW to add the sta
> 
> Then we have an endless loop of sta add/remove..

I'd wondered about that race condition before -- maybe hostapd should
add the station before sending the assoc complete. The race also exists
in practice with just mac80211, except it only leads to a dropped frame.

johannes

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Arik Nemtsov]

On Wed, Jan 26, 2011 at 23:54, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2011-01-26 at 23:44 +0200, Arik Nemtsov wrote:
>
>> Well no it doesn't do that. But the FW takes some time to know its
>> authenticated.
>> The flow is this:
>>
>> 1. hostapd sends auth complete (the FW doesn't know anything about this)
>
> you mean assoc, right?

Yea you're right. It's getting late I guess :)

>
>> 2. The STA transmits something
>> 3. The AP FW deauths the STA
>> 4. hostapd adds the station, causing mac80211 to call add_sta(), which
>> causes the FW to add the sta
>>
>> Then we have an endless loop of sta add/remove..
>
> I'd wondered about that race condition before -- maybe hostapd should
> add the station before sending the assoc complete. The race also exists
> in practice with just mac80211, except it only leads to a dropped frame.
>

Well it can still get dropped here. This just prevents the FW from
de-authenticating the STA.

Arik

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Johannes Berg]

On Thu, 2011-01-27 at 00:02 +0200, Arik Nemtsov wrote:

> >> 2. The STA transmits something
> >> 3. The AP FW deauths the STA
> >> 4. hostapd adds the station, causing mac80211 to call add_sta(), which
> >> causes the FW to add the sta
> >>
> >> Then we have an endless loop of sta add/remove..
> >
> > I'd wondered about that race condition before -- maybe hostapd should
> > add the station before sending the assoc complete. The race also exists
> > in practice with just mac80211, except it only leads to a dropped frame.
> 
> Well it can still get dropped here. This just prevents the FW from
> de-authenticating the STA.

No I mean the incoming frame would be dropped:

 - sta sends assoc
 - hostapd sends response
 - when assoc response ACK is received, hostapd sets state to assoc
 - incoming frame
 - hostapd adds STA to mac80211

then in this case mac80211 will pass the incoming frame up to hostapd,
and not to the network stack, but hostapd knows that it's a good frame
and won't kick out the station

The only difference is that with your firmware the kicking out is done
by the firmware, not hostapd, so it actually gets kicked out rather than
the frame being dropped.

So ... the race exists even without the firmware, it just has less
severe consequences.

If we add the station before the response frame's ACK is received, we
still have the race, and we need to delete if we don't get the ACK. If
we add the station before we send the assoc response we get rid of this
race, but also have to delete if we don't get the ACK. I guess that case
is actually nicer in some way since we would have allowed the station to
connect, so if spurious data frames go up because we don't get an ACK
from the station that's no big deal?

johannes

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Arik Nemtsov]

On Thu, Jan 27, 2011 at 00:08, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2011-01-27 at 00:02 +0200, Arik Nemtsov wrote:
>
>> >> 2. The STA transmits something
>> >> 3. The AP FW deauths the STA
>> >> 4. hostapd adds the station, causing mac80211 to call add_sta(), which
>> >> causes the FW to add the sta
>> >>
>> >> Then we have an endless loop of sta add/remove..
>> >
>> > I'd wondered about that race condition before -- maybe hostapd should
>> > add the station before sending the assoc complete. The race also exists
>> > in practice with just mac80211, except it only leads to a dropped frame.
>>
>> Well it can still get dropped here. This just prevents the FW from
>> de-authenticating the STA.
>
> No I mean the incoming frame would be dropped:

In my previous email what I meant was - even with this patch for the
FW (which allows the incoming packet through without the STA getting
de-authed), the incoming frame can still get dropped by mac80211.
The race always exists in mac80211/hostapd.

>
> If we add the station before the response frame's ACK is received, we
> still have the race, and we need to delete if we don't get the ACK. If
> we add the station before we send the assoc response we get rid of this
> race, but also have to delete if we don't get the ACK. I guess that case
> is actually nicer in some way since we would have allowed the station to
> connect, so if spurious data frames go up because we don't get an ACK
> from the station that's no big deal?
>

Maybe there's no need to delete the station even when no ACK is
received? It will get disconnected after the idle timeout anyway. And
it already passed authentication, so its not a DOS attack.
(If that's what you meant as well then I agree)

Arik

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Johannes Berg]

On Thu, 2011-01-27 at 00:16 +0200, Arik Nemtsov wrote:

> > If we add the station before the response frame's ACK is received, we
> > still have the race, and we need to delete if we don't get the ACK. If
> > we add the station before we send the assoc response we get rid of this
> > race, but also have to delete if we don't get the ACK. I guess that case
> > is actually nicer in some way since we would have allowed the station to
> > connect, so if spurious data frames go up because we don't get an ACK
> > from the station that's no big deal?
> >
> 
> Maybe there's no need to delete the station even when no ACK is
> received? It will get disconnected after the idle timeout anyway. And
> it already passed authentication, so its not a DOS attack.
> (If that's what you meant as well then I agree)

Yeah -- except there's no timeout in mac80211, of course, so hostapd
really has to delete it.

johannes

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Jouni Malinen]

On Wed, Jan 26, 2011 at 11:08:39PM +0100, Johannes Berg wrote:
> If we add the station before the response frame's ACK is received, we
> still have the race, and we need to delete if we don't get the ACK. If
> we add the station before we send the assoc response we get rid of this
> race, but also have to delete if we don't get the ACK. I guess that case
> is actually nicer in some way since we would have allowed the station to
> connect, so if spurious data frames go up because we don't get an ACK
> from the station that's no big deal?

>From the correctness view point, the STA entry would need to be enabled
(added/marked associated) at the moment the ACK frame is received. Since
we do some internal processing between kernel and user space, there is
obviously some latency involved. The most correct mechanism would be to
make hostapd add a not-associated-STA entry before sending association
response with status code 0 and request mac80211 to mark it associated
automatically on TX status report (showing success) from the driver.
However, for practical purposes, that may be too complex and doing what
you describe above is likely good enough.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 02/10] wl12xx: AP-mode - fix race condition on sta connection

[Johannes Berg]

On Thu, 2011-01-27 at 02:20 +0200, Jouni Malinen wrote:
> On Wed, Jan 26, 2011 at 11:08:39PM +0100, Johannes Berg wrote:
> > If we add the station before the response frame's ACK is received, we
> > still have the race, and we need to delete if we don't get the ACK. If
> > we add the station before we send the assoc response we get rid of this
> > race, but also have to delete if we don't get the ACK. I guess that case
> > is actually nicer in some way since we would have allowed the station to
> > connect, so if spurious data frames go up because we don't get an ACK
> > from the station that's no big deal?
> 
> From the correctness view point, the STA entry would need to be enabled
> (added/marked associated) at the moment the ACK frame is received. Since
> we do some internal processing between kernel and user space, there is
> obviously some latency involved. The most correct mechanism would be to
> make hostapd add a not-associated-STA entry before sending association
> response with status code 0 and request mac80211 to mark it associated
> automatically on TX status report (showing success) from the driver.
> However, for practical purposes, that may be too complex and doing what
> you describe above is likely good enough.

It's possible to do that as well -- but do we want to change any of
this? It might be good, but it's mostly a hostapd change I think?

johannes

Re: [PATCH 05/10] wl12xx: report invalid TX rate when returning non-TX-ed skbs

[Luciano Coelho]

On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
> Report a TX rate idx of -1 when returning untransmitted skbs to mac80211
> using ieee80211_tx_status(). Otherwise mac80211 tries to use the
> returned (essentially garbage) status.
> 
> Signed-off-by: Arik Nemtsov <arik@wizery.com>
> ---

Is this a documented way of doing it or it just happens to work?

-- 
Cheers,
Luca.

Re: [PATCH 05/10] wl12xx: report invalid TX rate when returning non-TX-ed skbs

[Johannes Berg]

On Thu, 2011-01-27 at 11:51 +0200, Luciano Coelho wrote:
> On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
> > Report a TX rate idx of -1 when returning untransmitted skbs to mac80211
> > using ieee80211_tx_status(). Otherwise mac80211 tries to use the
> > returned (essentially garbage) status.
> > 
> > Signed-off-by: Arik Nemtsov <arik@wizery.com>
> > ---
> 
> Is this a documented way of doing it or it just happens to work?

I think the more expected way would be to set the TX counts to 0?

johannes

Re: [PATCH 06/10] mac80211: add HW flag for disabling auto link-PS in AP mode

[Luciano Coelho]

On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
> When operating in AP mode the wl1271 hardware filters our null-data

s/our/out/

-- 
Cheers,
Luca.

Re: [PATCH 07/10] wl12xx: AP-mode - support HW based link PS monitoring

[Luciano Coelho]

On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
> When operating in AP mode the wl1271 hardware filters our null-data
> packets as well as management packets. This makes it impossible for
> mac80211 to monitor the PS mode by using the PM bit of incoming frames.

s/our/out/ here too.

-- 
Cheers,
Luca.

Re: [PATCH 05/10] wl12xx: report invalid TX rate when returning non-TX-ed skbs

[Arik Nemtsov]

On Thu, Jan 27, 2011 at 12:33, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2011-01-27 at 11:51 +0200, Luciano Coelho wrote:
>> On Sun, 2011-01-16 at 06:42 +0100, Arik Nemtsov wrote:
>> > Report a TX rate idx of -1 when returning untransmitted skbs to mac80211
>> > using ieee80211_tx_status(). Otherwise mac80211 tries to use the
>> > returned (essentially garbage) status.
>> >
>> > Signed-off-by: Arik Nemtsov <arik@wizery.com>
>> > ---
>>
>> Is this a documented way of doing it or it just happens to work?
>
> I think the more expected way would be to set the TX counts to 0?

Good catch. I'll set the rates[0].count = 0.

The original patch was to avoid this problematic line in ieee80211_tx_status:

		if ((local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL) &&
		    (rates_idx != -1))
			sta->last_tx_rate = info->status.rates[rates_idx];

If rates[0].idx is invalid this will be a bad deref.

Arik