[PATCH 1/6] mac80211: unset FC retry bit in mesh fwding path

[PATCH 1/6] mac80211: unset FC retry bit in mesh fwding path

[Thomas Pedersen]

Otherwise forwarded frames would keep the retry bit set
from the previous link transmission.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/rx.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5b4492a..5168f89 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2085,6 +2085,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 	}
 
 	fwd_hdr =  (struct ieee80211_hdr *) fwd_skb->data;
+	fwd_hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_RETRY);
 	info = IEEE80211_SKB_CB(fwd_skb);
 	memset(info, 0, sizeof(*info));
 	info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
-- 
1.7.10.4

[PATCH 2/6] mac80211: exclude multicast frames from BA accounting

[Thomas Pedersen]

Since multicast aggregation is not currently supported by
mac80211, we can safely ignore them when reordering
aggregate MPDUs.

This fixes a bug where the expected sequence number might
be reset after processing a multicast frame from a STA
with an established aggregation session. The expected
sequence number would then be incorrect and all aggregated
frames until the sequence number rolled over would be
dropped.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/rx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5168f89..cb55ef0 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -864,7 +864,8 @@ static void ieee80211_rx_reorder_ampdu(struct ieee80211_rx_data *rx,
 	u16 sc;
 	u8 tid, ack_policy;
 
-	if (!ieee80211_is_data_qos(hdr->frame_control))
+	if (!ieee80211_is_data_qos(hdr->frame_control) ||
+	    is_multicast_ether_addr(hdr->addr1))
 		goto dont_reorder;
 
 	/*
-- 
1.7.10.4

[PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Thomas Pedersen]

Have ieee80211_queue_stopped() return a bitmap of enum
queue_stop_reasons while checking queue status. This is
handy when we care about the queue stop reason codes.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 include/net/mac80211.h |   10 +++++++---
 net/mac80211/util.c    |    8 ++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 64faf01..8bbe799 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue);
 
 /**
  * ieee80211_queue_stopped - test status of the queue
+ *
  * @hw: pointer as obtained from ieee80211_alloc_hw().
  * @queue: queue number (counted from zero).
  *
  * Drivers should use this function instead of netif_stop_queue.
  *
- * Return: %true if the queue is stopped. %false otherwise.
+ * Return 0 if queue not stopped, or else a bitmap of
+ * queue_stop_reasons.
+ *
+ * If @queue doesn't exist, return -1UL.
+ *
  */
-
-int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue);
+unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue);
 
 /**
  * ieee80211_stop_queues - stop all queues
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 447e665..739cddb 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -475,17 +475,17 @@ void ieee80211_stop_queues(struct ieee80211_hw *hw)
 }
 EXPORT_SYMBOL(ieee80211_stop_queues);
 
-int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
+unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	unsigned long flags;
-	int ret;
+	unsigned long ret;
 
 	if (WARN_ON(queue >= hw->queues))
-		return true;
+		return -1UL;
 
 	spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
-	ret = !!local->queue_stop_reasons[queue];
+	ret = local->queue_stop_reasons[queue];
 	spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
 	return ret;
 }
-- 
1.7.10.4

[PATCH 4/6] mac80211: limit mesh forwarding drops

[Thomas Pedersen]

In case of the HW asking mac80211 to back off, the queues
would be stopped with reason DRIVER.

Limit dropping forwarded mesh data frames by putting them
on the pending queue anyway if the outgoing HW queues
appear to be only momentarily stopped.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/rx.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index cb55ef0..8c7f80c 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1997,6 +1997,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	__le16 reason = cpu_to_le16(WLAN_REASON_MESH_PATH_NOFORWARD);
 	u16 q, hdrlen;
+	unsigned long qreason;
 
 	hdr = (struct ieee80211_hdr *) skb->data;
 	hdrlen = ieee80211_hdrlen(hdr->frame_control);
@@ -2064,7 +2065,9 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 		return RX_CONTINUE;
 
 	q = ieee80211_select_queue_80211(sdata, skb, hdr);
-	if (ieee80211_queue_stopped(&local->hw, q)) {
+	qreason = ieee80211_queue_stopped(&local->hw, q);
+	if (qreason & ~(IEEE80211_QUEUE_STOP_REASON_SKB_ADD |
+			IEEE80211_QUEUE_STOP_REASON_AGGREGATION)) {
 		IEEE80211_IFSTA_MESH_CTR_INC(ifmsh, dropped_frames_congestion);
 		return RX_DROP_MONITOR;
 	}
-- 
1.7.10.4

[PATCH 5/6] mac80211: stringify another plink state

[Thomas Pedersen]

The patch "mac80211: stringify mesh peering events" missed
an opportunity to print the peering state as a string.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/mesh_plink.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 937e06f..cdd4183 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -544,8 +544,8 @@ static void mesh_plink_timer(unsigned long data)
 		return;
 	}
 	mpl_dbg(sta->sdata,
-		"Mesh plink timer for %pM fired on state %d\n",
-		sta->sta.addr, sta->plink_state);
+		"Mesh plink timer for %pM fired on state %s\n",
+		sta->sta.addr, mplstates[sta->plink_state]);
 	reason = 0;
 	llid = sta->llid;
 	plid = sta->plid;
-- 
1.7.10.4

[PATCH 6/6] mac80211: avoid mesh peer rate update warning

[Thomas Pedersen]

Some drivers (like ath9k_htc) will take a mutex in
rate_control_rate_update(), so drop the sta lock before we
get there.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>

Can go towards 3.9.
---
 net/mac80211/mesh_plink.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index cdd4183..ca284ee 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -379,8 +379,10 @@ static void mesh_sta_info_init(struct ieee80211_sub_if_data *sdata,
 	sta->last_rx = jiffies;
 
 	/* rates and capabilities don't change during peering */
-	if (sta->plink_state == NL80211_PLINK_ESTAB)
-		goto out;
+	if (sta->plink_state == NL80211_PLINK_ESTAB) {
+		spin_unlock_bh(&sta->lock);
+		return;
+	}
 
 	if (sta->sta.supp_rates[band] != rates)
 		changed |= IEEE80211_RC_SUPP_RATES_CHANGED;
@@ -398,13 +400,12 @@ static void mesh_sta_info_init(struct ieee80211_sub_if_data *sdata,
 			changed |= IEEE80211_RC_BW_CHANGED;
 		sta->sta.bandwidth = IEEE80211_STA_RX_BW_20;
 	}
+	spin_unlock_bh(&sta->lock);
 
 	if (insert)
 		rate_control_rate_init(sta);
 	else
 		rate_control_rate_update(local, sband, sta, changed);
-out:
-	spin_unlock_bh(&sta->lock);
 }
 
 static struct sta_info *
-- 
1.7.10.4

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Antonio Quartulli]

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

Hi Thomas,

On Mon, Apr 08, 2013 at 11:06:14AM -0700, Thomas Pedersen wrote:
> Have ieee80211_queue_stopped() return a bitmap of enum
> queue_stop_reasons while checking queue status. This is
> handy when we care about the queue stop reason codes.
> 
> Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
> ---
>  include/net/mac80211.h |   10 +++++++---
>  net/mac80211/util.c    |    8 ++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 64faf01..8bbe799 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue);
>  
>  /**
>   * ieee80211_queue_stopped - test status of the queue
> + *

I think you added this new line by mistake.

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Johannes Berg]

On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Have ieee80211_queue_stopped() return a bitmap of enum
> queue_stop_reasons while checking queue status. This is
> handy when we care about the queue stop reason codes.

Nack. The driver must not care about the reasons, they're purely
internal to mac80211.

johannes

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Thomas Pedersen]

On Mon, Apr 8, 2013 at 11:37 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
>> Have ieee80211_queue_stopped() return a bitmap of enum
>> queue_stop_reasons while checking queue status. This is
>> handy when we care about the queue stop reason codes.
>
> Nack. The driver must not care about the reasons, they're purely
> internal to mac80211.

OK those functions are exported to the drivers. The 'enum
queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
be able to interpret them anyway?

Would you prefer a utility function internal to mac80211 which does
return the reason, or just the following pattern?

if (ieee80211_queue_stopped(hw, queue)) {
    qreason = hw_to_local(hw)->queue_stop_reasons[queue];
    if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
        something;
}

Thanks,

-- 
Thomas

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Johannes Berg]

On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote:

> OK those functions are exported to the drivers. The 'enum
> queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
> be able to interpret them anyway?

Oh, you're way underestimating the creativity of driver authors :-)

> Would you prefer a utility function internal to mac80211 which does
> return the reason, 

That seems fine, although a bit more inefficient?

> or just the following pattern?
> 
> if (ieee80211_queue_stopped(hw, queue)) {
>     qreason = hw_to_local(hw)->queue_stop_reasons[queue];
>     if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
>         something;
> }

That seems racy? Even asking whether it's stopped is racy though, what
are you even trying to accomplish?

johannes

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Thomas Pedersen]

On Mon, Apr 8, 2013 at 12:38 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote:
>
>> OK those functions are exported to the drivers. The 'enum
>> queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
>> be able to interpret them anyway?
>
> Oh, you're way underestimating the creativity of driver authors :-)
>
>> Would you prefer a utility function internal to mac80211 which does
>> return the reason,
>
> That seems fine, although a bit more inefficient?
>
>> or just the following pattern?
>>
>> if (ieee80211_queue_stopped(hw, queue)) {
>>     qreason = hw_to_local(hw)->queue_stop_reasons[queue];
>>     if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
>>         something;
>> }
>
> That seems racy? Even asking whether it's stopped is racy though, what
> are you even trying to accomplish?

Yeah, that's why I'd like to get the reason the first time.

_h_mesh_fwding() checks whether the outgoing queue is stopped, to
avoid piling frames on the pending queue if the outgoing medium is
busy. I guess the idea was to avoid queueing frames faster than the
hardware could unload them. Maybe this doesn't actually happen, but we
can be a little bit smarter about when to drop forwarded frames. Like
if skbs are just being added to the outgoing queue, we probably
shouldn't.

This patch and the next one show a small improvement in throughput and
loss % in the forwarding path.

-- 
Thomas

Re: [PATCH 5/6] mac80211: stringify another plink state

[Joe Perches]

On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> The patch "mac80211: stringify mesh peering events" missed
> an opportunity to print the peering state as a string.

Perhaps it's better to use functions for these instead of
indexing in case an index is somehow incorrectly set.

> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
[]
> @@ -544,8 +544,8 @@ static void mesh_plink_timer(unsigned long data)
>  		return;
>  	}
>  	mpl_dbg(sta->sdata,
> -		"Mesh plink timer for %pM fired on state %d\n",
> -		sta->sta.addr, sta->plink_state);
> +		"Mesh plink timer for %pM fired on state %s\n",
> +		sta->sta.addr, mplstates[sta->plink_state]);

Re: [PATCH 5/6] mac80211: stringify another plink state

[Joe Perches]

On Mon, 2013-04-08 at 14:27 -0700, Joe Perches wrote:
> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> > The patch "mac80211: stringify mesh peering events" missed
> > an opportunity to print the peering state as a string.
> 
> Perhaps it's better to use functions for these instead of
> indexing in case an index is somehow incorrectly set.

Maybe:

 net/mac80211/mesh_plink.c | 73 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 937e06f..55d01d4 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -37,27 +37,55 @@ enum plink_event {
 	CLS_IGNR
 };
 
-static const char * const mplstates[] = {
-	[NL80211_PLINK_LISTEN] = "LISTEN",
-	[NL80211_PLINK_OPN_SNT] = "OPN-SNT",
-	[NL80211_PLINK_OPN_RCVD] = "OPN-RCVD",
-	[NL80211_PLINK_CNF_RCVD] = "CNF_RCVD",
-	[NL80211_PLINK_ESTAB] = "ESTAB",
-	[NL80211_PLINK_HOLDING] = "HOLDING",
-	[NL80211_PLINK_BLOCKED] = "BLOCKED"
-};
+static const char *mplstates(enum nl80211_plink_state state)
+{
+	switch (state) {
+	case NL80211_PLINK_LISTEN:
+		return "LISTEN";
+	case NL80211_PLINK_OPN_SNT:
+		return "OPN-SNT";
+	case NL80211_PLINK_OPN_RCVD:
+		return "OPN-RCVD";
+	case NL80211_PLINK_CNF_RCVD:
+		return "CNF_RCVD";
+	case NL80211_PLINK_ESTAB:
+		return "ESTAB";
+	case NL80211_PLINK_HOLDING:
+		return "HOLDING";
+	case NL80211_PLINK_BLOCKED:
+		return "BLOCKED";
+	default:
+		break;
+	}
+	return "UNKNOWN_PLINK_STATE";
+}
 
-static const char * const mplevents[] = {
-	[PLINK_UNDEFINED] = "NONE",
-	[OPN_ACPT] = "OPN_ACPT",
-	[OPN_RJCT] = "OPN_RJCT",
-	[OPN_IGNR] = "OPN_IGNR",
-	[CNF_ACPT] = "CNF_ACPT",
-	[CNF_RJCT] = "CNF_RJCT",
-	[CNF_IGNR] = "CNF_IGNR",
-	[CLS_ACPT] = "CLS_ACPT",
-	[CLS_IGNR] = "CLS_IGNR"
-};
+static const char *mplevents(enum plink_event event)
+{
+	switch (event) {
+	case PLINK_UNDEFINED:
+		return "NONE";
+	case OPN_ACPT:
+		return "OPN_ACPT";
+	case OPN_RJCT:
+		return "OPN_RJCT";
+	case OPN_IGNR:
+		return "OPN_IGNR";
+	case CNF_ACPT:
+		return "CNF_ACPT";
+	case CNF_RJCT:
+		return "CNF_RJCT";
+	case CNF_IGNR:
+		return "CNF_IGNR";
+	case CLS_ACPT:
+		return "CLS_ACPT";
+	case CLS_IGNR:
+		return "CLS_IGNR";
+	default:
+		break;
+	}
+	return "UNKNOWN_PLINK_EVENT";
+}
 
 static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 			       enum ieee80211_self_protected_actioncode action,
@@ -841,8 +869,9 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	mpl_dbg(sdata, "peer %pM in state %s got event %s\n", mgmt->sa,
-		       mplstates[sta->plink_state], mplevents[event]);
+	mpl_dbg(sdata, "peer %pM in state %d (%s) got event %d (%s)\n",
+		mgmt->sa, sta->plink_state, mplstates(sta->plink_state),
+		event, mplevents(event));
 	reason = 0;
 	spin_lock_bh(&sta->lock);
 	switch (sta->plink_state) {

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Johannes Berg]

On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote:

> > That seems racy? Even asking whether it's stopped is racy though, what
> > are you even trying to accomplish?
> 
> Yeah, that's why I'd like to get the reason the first time.
> 
> _h_mesh_fwding() checks whether the outgoing queue is stopped, to
> avoid piling frames on the pending queue if the outgoing medium is
> busy. I guess the idea was to avoid queueing frames faster than the
> hardware could unload them. Maybe this doesn't actually happen, but we
> can be a little bit smarter about when to drop forwarded frames. Like
> if skbs are just being added to the outgoing queue, we probably
> shouldn't.

Technically I guess that can happen if your inbound link is better than
the outbound one? But it'll depend on the AC parameters etc. too.

However it seems that ieee80211_queue_stopped() is actually kinda broken
and should only return the 'driver-stopped' reason to start with. If you
fix that, it's probably good enough for you. I don't think in patch 4
you should drop frames for scanning, for example.

johannes

Re: [PATCH 6/6] mac80211: avoid mesh peer rate update warning

[Johannes Berg]

On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Some drivers (like ath9k_htc) will take a mutex in
> rate_control_rate_update(), so drop the sta lock before we
> get there.

That driver is broken then. rate_control_rate_update() can be called
from the RX path where this also isn't allowed.

johannes

Re: [PATCH 1/6] mac80211: unset FC retry bit in mesh fwding path

[Johannes Berg]

On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Otherwise forwarded frames would keep the retry bit set
> from the previous link transmission.

Applied.

johannes

Re: [PATCH 6/6] mac80211: avoid mesh peer rate update warning

[Johannes Berg]

On Tue, 2013-04-09 at 11:38 +0200, Johannes Berg wrote:
> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> > Some drivers (like ath9k_htc) will take a mutex in
> > rate_control_rate_update(), so drop the sta lock before we
> > get there.
> 
> That driver is broken then. rate_control_rate_update() can be called
> from the RX path where this also isn't allowed.

 * @sta_rc_update: Notifies the driver of changes to the bitrates that
can be
 *      used to transmit to the station. The changes are advertised with
bits
 *      from &enum ieee80211_rate_control_changed and the values are
reflected
 *      in the station data. This callback should only be used when the
driver
 *      uses hardware rate control (%IEEE80211_HW_HAS_RATE_CONTROL)
since
 *      otherwise the rate control algorithm is notified directly.
 *      Must be atomic.

(note last line)

johannes

Re: [PATCH 2/6] mac80211: exclude multicast frames from BA accounting

[Johannes Berg]

Subject should probably say "reordering" rather than "accounting"

> Since multicast aggregation is not currently supported by
> mac80211, we can safely ignore them when reordering
> aggregate MPDUs.

Multicast may have a lower expectation of ordering than unicast, but is
this really the right thing to do? It seems better to just fix it?

johannes

Re: [PATCH 5/6] mac80211: stringify another plink state

[Johannes Berg]

On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> The patch "mac80211: stringify mesh peering events" missed
> an opportunity to print the peering state as a string.

Applied.

johannes

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Thomas Pedersen]

On Tue, Apr 9, 2013 at 2:37 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote:
>
>> > That seems racy? Even asking whether it's stopped is racy though, what
>> > are you even trying to accomplish?
>>
>> Yeah, that's why I'd like to get the reason the first time.
>>
>> _h_mesh_fwding() checks whether the outgoing queue is stopped, to
>> avoid piling frames on the pending queue if the outgoing medium is
>> busy. I guess the idea was to avoid queueing frames faster than the
>> hardware could unload them. Maybe this doesn't actually happen, but we
>> can be a little bit smarter about when to drop forwarded frames. Like
>> if skbs are just being added to the outgoing queue, we probably
>> shouldn't.
>
> Technically I guess that can happen if your inbound link is better than
> the outbound one? But it'll depend on the AC parameters etc. too.
>
> However it seems that ieee80211_queue_stopped() is actually kinda broken
> and should only return the 'driver-stopped' reason to start with. If you
> fix that, it's probably good enough for you. I don't think in patch 4
> you should drop frames for scanning, for example.

It looks like mac80211 already just checks local->queue_stop_reasons
for being 0 (or not), so that check would still hold. I'm not 100%
sure the driver calls to ieee80211_queue_stopped() only returning true
for REASON_DRIVER are ok? What if the queues are being flushed, or say
REASON_PS is set?

-- 
Thomas

Re: [PATCH 6/6] mac80211: avoid mesh peer rate update warning

[Thomas Pedersen]

On Tue, Apr 9, 2013 at 2:42 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2013-04-09 at 11:38 +0200, Johannes Berg wrote:
>> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
>> > Some drivers (like ath9k_htc) will take a mutex in
>> > rate_control_rate_update(), so drop the sta lock before we
>> > get there.
>>
>> That driver is broken then. rate_control_rate_update() can be called
>> from the RX path where this also isn't allowed.
>
>  * @sta_rc_update: Notifies the driver of changes to the bitrates that
> can be
>  *      used to transmit to the station. The changes are advertised with
> bits
>  *      from &enum ieee80211_rate_control_changed and the values are
> reflected
>  *      in the station data. This callback should only be used when the
> driver
>  *      uses hardware rate control (%IEEE80211_HW_HAS_RATE_CONTROL)
> since
>  *      otherwise the rate control algorithm is notified directly.
>  *      Must be atomic.
>
> (note last line)

Ah alright. Thanks.

-- 
Thomas

Re: [PATCH 3/6] mac80211: ieee80211_queue_stopped returns reasons

[Johannes Berg]

On Wed, 2013-04-10 at 10:36 -0700, Thomas Pedersen wrote:

> > However it seems that ieee80211_queue_stopped() is actually kinda broken
> > and should only return the 'driver-stopped' reason to start with. If you
> > fix that, it's probably good enough for you. I don't think in patch 4
> > you should drop frames for scanning, for example.
> 
> It looks like mac80211 already just checks local->queue_stop_reasons
> for being 0 (or not), so that check would still hold. I'm not 100%
> sure the driver calls to ieee80211_queue_stopped() only returning true
> for REASON_DRIVER are ok? What if the queues are being flushed, or say
> REASON_PS is set?

I looked at the drivers, and while (almost?) all of them use it
incorrectly they really want only REASON_DRIVER. In fact, they'd be
better off with just that even in the incorrect usages.

johannes

Re: [PATCH 2/6] mac80211: exclude multicast frames from BA accounting

[Thomas Pedersen]

On Tue, Apr 9, 2013 at 2:47 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Subject should probably say "reordering" rather than "accounting"
>
>> Since multicast aggregation is not currently supported by
>> mac80211, we can safely ignore them when reordering
>> aggregate MPDUs.
>
> Multicast may have a lower expectation of ordering than unicast, but is
> this really the right thing to do? It seems better to just fix it?

It appears one of our changes actually introduced this bug!  It is not
reproducible with mac80211-next. Thanks for making me take a closer
look at this.

-- 
Thomas