[PATCH 1/2] mac80211: allow low level drivers to report packet loss

[PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Arik Nemtsov]

Add API that allows low level drivers to notify mac80211 about TX
packet loss. This is useful when there are FW triggers to notify the
low level driver about these events.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 include/net/mac80211.h |   11 +++++++++++
 net/mac80211/status.c  |    8 ++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cefe1b3..efbdda9 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2265,6 +2265,17 @@ void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
 				 struct sk_buff *skb);
 
 /**
+ * ieee80211_report_low_ack - report non-responding station
+ *
+ * When operating in AP-mode, call this function to report a non-responding
+ * connected STA.
+ *
+ * @sta: the non-responding connected sta
+ * @num_packets: number of packets sent to @sta without a response
+ */
+void ieee80211_report_low_ack(struct ieee80211_sta *sta, u32 num_packets);
+
+/**
  * ieee80211_beacon_get_tim - beacon generation function
  * @hw: pointer obtained from ieee80211_alloc_hw().
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 3ed3c83..1658efa 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -446,3 +446,11 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	dev_kfree_skb(skb);
 }
 EXPORT_SYMBOL(ieee80211_tx_status);
+
+void ieee80211_report_low_ack(struct ieee80211_sta *pubsta, u32 num_packets)
+{
+	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+	cfg80211_cqm_pktloss_notify(sta->sdata->dev, sta->sta.addr,
+				    num_packets, GFP_ATOMIC);
+}
+EXPORT_SYMBOL(ieee80211_report_low_ack);
-- 
1.7.1

[PATCH 2/2] wl12xx: support FW TX inactivity triggers

[Arik Nemtsov]

In AP mode register for the MAX_TX_RETRY and INACTIVE_STA events.
Both are reported to the upper layers as a TX failure in the offending
stations.

In STA mode we register only for the MAX_TX_RETRY event. A TX failure is
interpreted as a loss of connection.

Support for IEEE80211_HW_REPORTS_TX_ACK_STATUS has been removed to avoid
the inherent race condition of a mac80211 TX failure counter in addition
to the FW counter.

This patch depends on "mac80211: allow low level drivers to report packet
loss"

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/wl12xx/boot.c   |    6 +++-
 drivers/net/wireless/wl12xx/cmd.c    |    2 +-
 drivers/net/wireless/wl12xx/conf.h   |    6 ++++
 drivers/net/wireless/wl12xx/event.c  |   47 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/event.h  |   12 ++++++++-
 drivers/net/wireless/wl12xx/main.c   |    8 +++++-
 drivers/net/wireless/wl12xx/wl12xx.h |    1 -
 7 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/boot.c b/drivers/net/wireless/wl12xx/boot.c
index b5ec2c2..072f783 100644
--- a/drivers/net/wireless/wl12xx/boot.c
+++ b/drivers/net/wireless/wl12xx/boot.c
@@ -478,10 +478,12 @@ static int wl1271_boot_run_firmware(struct wl1271 *wl)
 		DISCONNECT_EVENT_COMPLETE_ID |
 		RSSI_SNR_TRIGGER_0_EVENT_ID |
 		PSPOLL_DELIVERY_FAILURE_EVENT_ID |
-		SOFT_GEMINI_SENSE_EVENT_ID;
+		SOFT_GEMINI_SENSE_EVENT_ID |
+		MAX_TX_RETRY_EVENT_ID;
 
 	if (wl->bss_type == BSS_TYPE_AP_BSS)
-		wl->event_mask |= STA_REMOVE_COMPLETE_EVENT_ID;
+		wl->event_mask |= STA_REMOVE_COMPLETE_EVENT_ID |
+				  INACTIVE_STA_EVENT_ID;
 	else
 		wl->event_mask |= DUMMY_PACKET_EVENT_ID;
 
diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
index 2468044..d483316 100644
--- a/drivers/net/wireless/wl12xx/cmd.c
+++ b/drivers/net/wireless/wl12xx/cmd.c
@@ -1070,7 +1070,7 @@ int wl1271_cmd_start_bss(struct wl1271 *wl)
 
 	memcpy(cmd->bssid, bss_conf->bssid, ETH_ALEN);
 
-	cmd->aging_period = cpu_to_le16(WL1271_AP_DEF_INACTIV_SEC);
+	cmd->aging_period = cpu_to_le16(wl->conf.tx.ap_aging_period);
 	cmd->bss_index = WL1271_AP_BSS_INDEX;
 	cmd->global_hlid = WL1271_AP_GLOBAL_HLID;
 	cmd->broadcast_hlid = WL1271_AP_BROADCAST_HLID;
diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
index 743bd0b..e6398e0 100644
--- a/drivers/net/wireless/wl12xx/conf.h
+++ b/drivers/net/wireless/wl12xx/conf.h
@@ -689,6 +689,12 @@ struct conf_tx_settings {
 	u16 ap_max_tx_retries;
 
 	/*
+	 * AP-mode - after this number of seconds a connected station is
+	 * considered inactive.
+	 */
+	u16 ap_aging_period;
+
+	/*
 	 * Configuration for TID parameters.
 	 */
 	u8 tid_conf_count;
diff --git a/drivers/net/wireless/wl12xx/event.c b/drivers/net/wireless/wl12xx/event.c
index ae69330..2d484db 100644
--- a/drivers/net/wireless/wl12xx/event.c
+++ b/drivers/net/wireless/wl12xx/event.c
@@ -174,6 +174,8 @@ static int wl1271_event_process(struct wl1271 *wl, struct event_mailbox *mbox)
 	u32 vector;
 	bool beacon_loss = false;
 	bool is_ap = (wl->bss_type == BSS_TYPE_AP_BSS);
+	bool disconnect_sta = false;
+	unsigned long sta_bitmap = 0;
 
 	wl1271_event_mbox_dump(mbox);
 
@@ -235,9 +237,54 @@ static int wl1271_event_process(struct wl1271 *wl, struct event_mailbox *mbox)
 			wl1271_tx_dummy_packet(wl);
 	}
 
+	/*
+	 * "TX retries exceeded" has a different meaning according to mode.
+	 * In AP mode the offending station is disconnected. In STA mode we
+	 * report connection loss.
+	 */
+	if (vector & MAX_TX_RETRY_EVENT_ID) {
+		wl1271_debug(DEBUG_EVENT, "MAX_TX_RETRY_EVENT_ID");
+		if (is_ap) {
+			sta_bitmap |= le16_to_cpu(mbox->sta_tx_retry_exceeded);
+			disconnect_sta = true;
+		} else {
+			beacon_loss = true;
+		}
+	}
+
+	if ((vector & INACTIVE_STA_EVENT_ID) && is_ap) {
+		wl1271_debug(DEBUG_EVENT, "INACTIVE_STA_EVENT_ID");
+		sta_bitmap |= le16_to_cpu(mbox->sta_aging_status);
+		disconnect_sta = true;
+	}
+
 	if (wl->vif && beacon_loss)
 		ieee80211_connection_loss(wl->vif);
 
+	if (is_ap && disconnect_sta) {
+		u32 num_packets = wl->conf.tx.ap_max_tx_retries;
+		struct ieee80211_sta *sta;
+		const u8 *addr;
+		int h;
+
+		for (h = find_first_bit(&sta_bitmap, AP_MAX_LINKS);
+		     h < AP_MAX_LINKS;
+		     h = find_next_bit(&sta_bitmap, AP_MAX_LINKS, h+1)) {
+			if (!wl1271_is_active_sta(wl, h))
+				continue;
+
+			addr = wl->links[h].addr;
+
+			rcu_read_lock();
+			sta = ieee80211_find_sta(wl->vif, addr);
+			if (sta) {
+				wl1271_debug(DEBUG_EVENT, "remove sta %d", h);
+				ieee80211_report_low_ack(sta, num_packets);
+			}
+			rcu_read_unlock();
+		}
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/wl12xx/event.h b/drivers/net/wireless/wl12xx/event.h
index b6cf06e..7ae5a08 100644
--- a/drivers/net/wireless/wl12xx/event.h
+++ b/drivers/net/wireless/wl12xx/event.h
@@ -58,13 +58,16 @@ enum {
 	CHANNEL_SWITCH_COMPLETE_EVENT_ID	 = BIT(17),
 	BSS_LOSE_EVENT_ID			 = BIT(18),
 	REGAINED_BSS_EVENT_ID			 = BIT(19),
-	ROAMING_TRIGGER_MAX_TX_RETRY_EVENT_ID	 = BIT(20),
+	MAX_TX_RETRY_EVENT_ID			 = BIT(20),
 	/* STA: dummy paket for dynamic mem blocks */
 	DUMMY_PACKET_EVENT_ID                    = BIT(21),
 	/* AP: STA remove complete */
 	STA_REMOVE_COMPLETE_EVENT_ID             = BIT(21),
 	SOFT_GEMINI_SENSE_EVENT_ID		 = BIT(22),
+	/* STA: SG prediction */
 	SOFT_GEMINI_PREDICTION_EVENT_ID		 = BIT(23),
+	/* AP: Inactive STA */
+	INACTIVE_STA_EVENT_ID			 = BIT(23),
 	SOFT_GEMINI_AVALANCHE_EVENT_ID		 = BIT(24),
 	PLT_RX_CALIBRATION_COMPLETE_EVENT_ID	 = BIT(25),
 	DBG_EVENT_ID				 = BIT(26),
@@ -119,7 +122,11 @@ struct event_mailbox {
 
 	/* AP FW only */
 	u8 hlid_removed;
+
+	/* a bitmap of hlids for stations that have been inactive too long */
 	__le16 sta_aging_status;
+
+	/* a bitmap of hlids for stations which didn't respond to TX */
 	__le16 sta_tx_retry_exceeded;
 
 	u8 reserved_5[24];
@@ -130,4 +137,7 @@ void wl1271_event_mbox_config(struct wl1271 *wl);
 int wl1271_event_handle(struct wl1271 *wl, u8 mbox);
 void wl1271_pspoll_work(struct work_struct *work);
 
+/* Functions from main.c */
+bool wl1271_is_active_sta(struct wl1271 *wl, u8 hlid);
+
 #endif
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 7126506..e840176 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -193,6 +193,7 @@ static struct conf_drv_settings default_conf = {
 			.aflags              = 0,
 		},
 		.ap_max_tx_retries = 100,
+		.ap_aging_period = 300,
 		.tid_conf_count = 4,
 		.tid_conf = {
 			[CONF_TX_AC_BE] = {
@@ -2952,6 +2953,12 @@ static void wl1271_free_sta(struct wl1271 *wl, u8 hlid)
 	__clear_bit(hlid, (unsigned long *)&wl->ap_fw_ps_map);
 }
 
+bool wl1271_is_active_sta(struct wl1271 *wl, u8 hlid)
+{
+	int id = hlid - WL1271_AP_STA_HLID_START;
+	return test_bit(id, wl->ap_hlid_map);
+}
+
 static int wl1271_op_sta_add(struct ieee80211_hw *hw,
 			     struct ieee80211_vif *vif,
 			     struct ieee80211_sta *sta)
@@ -3535,7 +3542,6 @@ int wl1271_init_ieee80211(struct wl1271 *wl)
 		IEEE80211_HW_HAS_RATE_CONTROL |
 		IEEE80211_HW_CONNECTION_MONITOR |
 		IEEE80211_HW_SUPPORTS_CQM_RSSI |
-		IEEE80211_HW_REPORTS_TX_ACK_STATUS |
 		IEEE80211_HW_AP_LINK_PS;
 
 	wl->hw->wiphy->cipher_suites = cipher_suites;
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index fb2b79f..7c521af 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -172,7 +172,6 @@ extern u32 wl12xx_debug_level;
 #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
 
 #define ACX_TX_DESCRIPTORS         32
-- 
1.7.1

Re: [PATCH 2/2] wl12xx: support FW TX inactivity triggers

[Arik Nemtsov]

On Mon, Apr 18, 2011 at 14:22, Arik Nemtsov <arik@wizery.com> wrote:
> In AP mode register for the MAX_TX_RETRY and INACTIVE_STA events.
> Both are reported to the upper layers as a TX failure in the offending
> stations.
>
> In STA mode we register only for the MAX_TX_RETRY event. A TX failure is
> interpreted as a loss of connection.
>
> Support for IEEE80211_HW_REPORTS_TX_ACK_STATUS has been removed to avoid
> the inherent race condition of a mac80211 TX failure counter in addition
> to the FW counter.
>
> This patch depends on "mac80211: allow low level drivers to report packet
> loss"
>
> Signed-off-by: Arik Nemtsov <arik@wizery.com>
> ---

It seems there's a bug in this patch. I'll send a v2 shortly. Please
hold off on the review for now.

Thanks,
Arik

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Johannes Berg]

On Mon, 2011-04-18 at 14:22 +0300, Arik Nemtsov wrote:
> Add API that allows low level drivers to notify mac80211 about TX
> packet loss. This is useful when there are FW triggers to notify the
> low level driver about these events.

> +void ieee80211_report_low_ack(struct ieee80211_sta *pubsta, u32 num_packets)
> +{
> +	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
> +	cfg80211_cqm_pktloss_notify(sta->sdata->dev, sta->sta.addr,
> +				    num_packets, GFP_ATOMIC);

I think it would make more sense to add num_packets to sta->lost_packets
and invoke it only when over the threshold as done in
ieee80211_tx_status(), as then we can share any threshold calculation
improvements.

Also, are you sure? Do you really not get _any_ TX status?

johannes

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Arik Nemtsov]

On Mon, Apr 18, 2011 at 15:26, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2011-04-18 at 14:22 +0300, Arik Nemtsov wrote:
>> Add API that allows low level drivers to notify mac80211 about TX
>> packet loss. This is useful when there are FW triggers to notify the
>> low level driver about these events.
>
>> +void ieee80211_report_low_ack(struct ieee80211_sta *pubsta, u32 num_packets)
>> +{
>> +     struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
>> +     cfg80211_cqm_pktloss_notify(sta->sdata->dev, sta->sta.addr,
>> +                                 num_packets, GFP_ATOMIC);
>
> I think it would make more sense to add num_packets to sta->lost_packets
> and invoke it only when over the threshold as done in
> ieee80211_tx_status(), as then we can share any threshold calculation
> improvements.

Well in a wl12xx chip we just configure some thresholds to the FW and
it triggers the event for us. Using TX status doesn't really make
sense since the FW has automatic rate control (so 50 tries are more
like 50 * 8).

>
> Also, are you sure? Do you really not get _any_ TX status?

Currently we do get TX status, but there are plans to remove it later on.

Regards,
Arik

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Johannes Berg]

On Mon, 2011-04-18 at 23:44 +0300, Arik Nemtsov wrote:

> Well in a wl12xx chip we just configure some thresholds to the FW and
> it triggers the event for us. Using TX status doesn't really make
> sense since the FW has automatic rate control (so 50 tries are more
> like 50 * 8).

Yeah same for the mac80211 approach, since we take packets, not retries.

> > Also, are you sure? Do you really not get _any_ TX status?
> 
> Currently we do get TX status, but there are plans to remove it later on.

That's your choice, but I wouldn't do it. You'll have to support TX
status when requested, otherwise AP operation won't work, so you need
the code anyway.

johannes

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Arik Nemtsov]

On Tue, Apr 19, 2011 at 15:36, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2011-04-18 at 23:44 +0300, Arik Nemtsov wrote:
>
>> Well in a wl12xx chip we just configure some thresholds to the FW and
>> it triggers the event for us. Using TX status doesn't really make
>> sense since the FW has automatic rate control (so 50 tries are more
>> like 50 * 8).
>
> Yeah same for the mac80211 approach, since we take packets, not retries.
>
>> > Also, are you sure? Do you really not get _any_ TX status?
>>
>> Currently we do get TX status, but there are plans to remove it later on.
>
> That's your choice, but I wouldn't do it. You'll have to support TX
> status when requested, otherwise AP operation won't work, so you need
> the code anyway.

Can you elaborate why?

I'm assuming you mean the removal of
IEEE80211_HW_REPORTS_TX_ACK_STATUS (in the second patch of this
series). Note that it was only added recently, and AP/STA modes seemed
to work fine without it.
>From a look in the code it seems this flag helps with connection
monitoring in STA mode. The other use is determining the current PS
mode when IEEE80211_HW_PS_NULLFUNC_STACK is enabled. These two are
done by HW in wl12xx cards.

Regards,
Arik

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Johannes Berg]

On Wed, 2011-04-20 at 00:54 +0300, Arik Nemtsov wrote:

> > That's your choice, but I wouldn't do it. You'll have to support TX
> > status when requested, otherwise AP operation won't work, so you need
> > the code anyway.
> 
> Can you elaborate why?
> 
> I'm assuming you mean the removal of
> IEEE80211_HW_REPORTS_TX_ACK_STATUS (in the second patch of this
> series). Note that it was only added recently, and AP/STA modes seemed
> to work fine without it.
> From a look in the code it seems this flag helps with connection
> monitoring in STA mode. The other use is determining the current PS
> mode when IEEE80211_HW_PS_NULLFUNC_STACK is enabled. These two are
> done by HW in wl12xx cards.

Well, there's the flag saying "you can rely on it", but there's also the
fact that AP mode relies on status for (some) frames anyway. So if you
just want to remove the flag I guess that's OK, but if you want to
remove all status processing .. that'll cause issues with AP mode.

johannes

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[John W. Linville]

On Wed, Apr 20, 2011 at 10:08:53AM +0200, Johannes Berg wrote:
> On Wed, 2011-04-20 at 00:54 +0300, Arik Nemtsov wrote:
> 
> > > That's your choice, but I wouldn't do it. You'll have to support TX
> > > status when requested, otherwise AP operation won't work, so you need
> > > the code anyway.
> > 
> > Can you elaborate why?
> > 
> > I'm assuming you mean the removal of
> > IEEE80211_HW_REPORTS_TX_ACK_STATUS (in the second patch of this
> > series). Note that it was only added recently, and AP/STA modes seemed
> > to work fine without it.
> > From a look in the code it seems this flag helps with connection
> > monitoring in STA mode. The other use is determining the current PS
> > mode when IEEE80211_HW_PS_NULLFUNC_STACK is enabled. These two are
> > done by HW in wl12xx cards.
> 
> Well, there's the flag saying "you can rely on it", but there's also the
> fact that AP mode relies on status for (some) frames anyway. So if you
> just want to remove the flag I guess that's OK, but if you want to
> remove all status processing .. that'll cause issues with AP mode.

Do the wl12xx guys still want this?

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 1/2] mac80211: allow low level drivers to report packet loss

[Arik Nemtsov]

On Tue, Apr 26, 2011 at 22:12, John W. Linville <linville@tuxdriver.com> wrote:
> On Wed, Apr 20, 2011 at 10:08:53AM +0200, Johannes Berg wrote:
>> On Wed, 2011-04-20 at 00:54 +0300, Arik Nemtsov wrote:
>>
>> > > That's your choice, but I wouldn't do it. You'll have to support TX
>> > > status when requested, otherwise AP operation won't work, so you need
>> > > the code anyway.
>> >
>> > Can you elaborate why?
>> >
>> > I'm assuming you mean the removal of
>> > IEEE80211_HW_REPORTS_TX_ACK_STATUS (in the second patch of this
>> > series). Note that it was only added recently, and AP/STA modes seemed
>> > to work fine without it.
>> > From a look in the code it seems this flag helps with connection
>> > monitoring in STA mode. The other use is determining the current PS
>> > mode when IEEE80211_HW_PS_NULLFUNC_STACK is enabled. These two are
>> > done by HW in wl12xx cards.
>>
>> Well, there's the flag saying "you can rely on it", but there's also the
>> fact that AP mode relies on status for (some) frames anyway. So if you
>> just want to remove the flag I guess that's OK, but if you want to
>> remove all status processing .. that'll cause issues with AP mode.
>
> Do the wl12xx guys still want this?
>

Yes definitely.

The above is a discussion about providing reliable TX status to
certain packets. It's not really related.
But Johannes has a point here and it was noted.

Regards,
Arik