[PATCH 0/2] QoS headers for mesh

[PATCH 0/2] QoS headers for mesh

[Javier Cardona]

Mesh frames are required to have QoS headers to indicate the presence of a Mesh
Control Header in the payload.  These patches add QoS headers to mesh frames,
but note that they don't implement full QoS support: mesh stations don't
currently advertise QoS capabilities.

Javier Cardona (2):
  mac80211: Start implementing QoS support for mesh interfaces
  mac80211: Mesh data frames must have the QoS header

 include/linux/ieee80211.h   |    2 ++
 net/mac80211/mesh.c         |    8 ++++----
 net/mac80211/mesh_pathtbl.c |    4 ++++
 net/mac80211/rx.c           |    6 +++---
 net/mac80211/tx.c           |    5 +++--
 net/mac80211/wme.c          |   15 ++++++---------
 net/mac80211/wme.h          |    2 +-
 7 files changed, 23 insertions(+), 19 deletions(-)

-- 
1.7.6

[PATCH 1/2] mac80211: Start implementing QoS support for mesh interfaces

[Javier Cardona]

In order to support QoS in mesh, we need to assign queue mapping only
after the next hop has been resolved, both for forwarded and locally
originated frames.  Also, now that this is fixed, remove the XXX comment
in ieee80211_select_queue().

Also, V-Shy Ho reported that the queue mapping was not being applied to
the forwarded frame (fwd_skb instead of skb).  Fixed that as well.

Signed-off-by: Javier Cardona <javier@cozybit.com>
---
 net/mac80211/mesh_pathtbl.c |    4 ++++
 net/mac80211/rx.c           |    6 +++---
 net/mac80211/wme.c          |    6 +-----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index ede4f52..b51fce6 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -14,6 +14,7 @@
 #include <linux/spinlock.h>
 #include <linux/string.h>
 #include <net/mac80211.h>
+#include "wme.h"
 #include "ieee80211_i.h"
 #include "mesh.h"
 
@@ -210,6 +211,7 @@ void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
 	struct ieee80211_hdr *hdr;
 	struct sk_buff_head tmpq;
 	unsigned long flags;
+	struct ieee80211_sub_if_data *sdata = mpath->sdata;
 
 	rcu_assign_pointer(mpath->next_hop, sta);
 
@@ -220,6 +222,8 @@ void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
 	while ((skb = __skb_dequeue(&mpath->frame_queue)) != NULL) {
 		hdr = (struct ieee80211_hdr *) skb->data;
 		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
+		skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
+		ieee80211_set_qos_hdr(sdata->local, skb);
 		__skb_queue_tail(&tmpq, skb);
 	}
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index f45fd2f..c5be3ef 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1902,9 +1902,6 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 			memset(info, 0, sizeof(*info));
 			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
 			info->control.vif = &rx->sdata->vif;
-			skb_set_queue_mapping(skb,
-				ieee80211_select_queue(rx->sdata, fwd_skb));
-			ieee80211_set_qos_hdr(local, skb);
 			if (is_multicast_ether_addr(fwd_hdr->addr1))
 				IEEE80211_IFSTA_MESH_CTR_INC(&sdata->u.mesh,
 								fwded_mcast);
@@ -1928,6 +1925,9 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 			}
 			IEEE80211_IFSTA_MESH_CTR_INC(&sdata->u.mesh,
 						     fwded_frames);
+			skb_set_queue_mapping(fwd_skb,
+				ieee80211_select_queue(rx->sdata, fwd_skb));
+			ieee80211_set_qos_hdr(local, fwd_skb);
 			ieee80211_add_pending_skb(local, fwd_skb);
 		}
 	}
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 7a49532..a9fee2b 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -83,11 +83,7 @@ u16 ieee80211_select_queue(struct ieee80211_sub_if_data *sdata,
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
-		/*
-		 * XXX: This is clearly broken ... but already was before,
-		 * because ieee80211_fill_mesh_addresses() would clear A1
-		 * except for multicast addresses.
-		 */
+		ra = skb->data;
 		break;
 #endif
 	case NL80211_IFTYPE_STATION:
-- 
1.7.6

[PATCH 2/2] mac80211: Mesh data frames must have the QoS header

[Javier Cardona]

Per sec 7.1.3.5 of draft 12.0 of 802.11s, mesh frames indicate the
presence of the mesh control header in their QoS header.

Signed-off-by: Javier Cardona <javier@cozybit.com>
---
 include/linux/ieee80211.h   |    2 ++
 net/mac80211/mesh.c         |    9 +++++----
 net/mac80211/mesh_pathtbl.c |    2 +-
 net/mac80211/rx.c           |    2 +-
 net/mac80211/tx.c           |    5 +++--
 net/mac80211/wme.c          |   10 ++++++----
 net/mac80211/wme.h          |    3 ++-
 7 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 37f95f2..72f3933 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -130,6 +130,8 @@
 #define IEEE80211_QOS_CTL_ACK_POLICY_BLOCKACK	0x0060
 /* A-MSDU 802.11n */
 #define IEEE80211_QOS_CTL_A_MSDU_PRESENT	0x0080
+/* Mesh Control 802.11s */
+#define IEEE80211_QOS_CTL_MESH_CONTROL_PRESENT  0x0100
 
 /* U-APSD queue for WMM IEs sent by AP */
 #define IEEE80211_WMM_IE_AP_QOSINFO_UAPSD	(1<<7)
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 28ab510..46f2b60b 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -457,21 +457,22 @@ int ieee80211_fill_mesh_addresses(struct ieee80211_hdr *hdr, __le16 *fc,
 				  const u8 *meshda, const u8 *meshsa)
 {
 	if (is_multicast_ether_addr(meshda)) {
-		*fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
+		*fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS |
+				   IEEE80211_STYPE_QOS_DATA);
 		/* DA TA SA */
 		memcpy(hdr->addr1, meshda, ETH_ALEN);
 		memcpy(hdr->addr2, meshsa, ETH_ALEN);
 		memcpy(hdr->addr3, meshsa, ETH_ALEN);
-		return 24;
+		return 26;
 	} else {
 		*fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS |
-				IEEE80211_FCTL_TODS);
+				IEEE80211_FCTL_TODS | IEEE80211_STYPE_QOS_DATA);
 		/* RA TA DA SA */
 		memset(hdr->addr1, 0, ETH_ALEN);   /* RA is resolved later */
 		memcpy(hdr->addr2, meshsa, ETH_ALEN);
 		memcpy(hdr->addr3, meshda, ETH_ALEN);
 		memcpy(hdr->addr4, meshsa, ETH_ALEN);
-		return 30;
+		return 32;
 	}
 }
 
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index b51fce6..8ac712b 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -223,7 +223,7 @@ void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
 		hdr = (struct ieee80211_hdr *) skb->data;
 		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
 		skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
-		ieee80211_set_qos_hdr(sdata->local, skb);
+		ieee80211_set_qos_hdr(sdata, skb);
 		__skb_queue_tail(&tmpq, skb);
 	}
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c5be3ef..c843795 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1927,7 +1927,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 						     fwded_frames);
 			skb_set_queue_mapping(fwd_skb,
 				ieee80211_select_queue(rx->sdata, fwd_skb));
-			ieee80211_set_qos_hdr(local, fwd_skb);
+			ieee80211_set_qos_hdr(rx->sdata, fwd_skb);
 			ieee80211_add_pending_skb(local, fwd_skb);
 		}
 	}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0107263..56da02c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1595,7 +1595,7 @@ static void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
 				return;
 			}
 
-	ieee80211_set_qos_hdr(local, skb);
+	ieee80211_set_qos_hdr(sdata, skb);
 	ieee80211_tx(sdata, skb, false);
 	rcu_read_unlock();
 }
@@ -1879,7 +1879,8 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 	}
 
 	/* receiver and we are QoS enabled, use a QoS type frame */
-	if ((sta_flags & WLAN_STA_WME) && local->hw.queues >= 4) {
+	if (!ieee80211_vif_is_mesh(&sdata->vif) && (sta_flags & WLAN_STA_WME)
+			&& local->hw.queues >= 4) {
 		fc |= cpu_to_le16(IEEE80211_STYPE_QOS_DATA);
 		hdrlen += 2;
 	}
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index a9fee2b..971004c 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -135,7 +135,8 @@ u16 ieee80211_downgrade_queue(struct ieee80211_local *local,
 	return ieee802_1d_to_ac[skb->priority];
 }
 
-void ieee80211_set_qos_hdr(struct ieee80211_local *local, struct sk_buff *skb)
+void ieee80211_set_qos_hdr(struct ieee80211_sub_if_data *sdata,
+			   struct sk_buff *skb)
 {
 	struct ieee80211_hdr *hdr = (void *)skb->data;
 
@@ -146,10 +147,11 @@ void ieee80211_set_qos_hdr(struct ieee80211_local *local, struct sk_buff *skb)
 
 		tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 
-		if (unlikely(local->wifi_wme_noack_test))
+		if (unlikely(sdata->local->wifi_wme_noack_test))
 			ack_policy |= IEEE80211_QOS_CTL_ACK_POLICY_NOACK;
-		/* qos header is 2 bytes, second reserved */
+		/* qos header is 2 bytes */
 		*p++ = ack_policy | tid;
-		*p = 0;
+		*p = ieee80211_vif_is_mesh(&sdata->vif) ?
+			(IEEE80211_QOS_CTL_MESH_CONTROL_PRESENT >> 8) : 0;
 	}
 }
diff --git a/net/mac80211/wme.h b/net/mac80211/wme.h
index faead6d..34e166f 100644
--- a/net/mac80211/wme.h
+++ b/net/mac80211/wme.h
@@ -17,7 +17,8 @@ extern const int ieee802_1d_to_ac[8];
 
 u16 ieee80211_select_queue(struct ieee80211_sub_if_data *sdata,
 			   struct sk_buff *skb);
-void ieee80211_set_qos_hdr(struct ieee80211_local *local, struct sk_buff *skb);
+void ieee80211_set_qos_hdr(struct ieee80211_sub_if_data *sdata,
+			   struct sk_buff *skb);
 u16 ieee80211_downgrade_queue(struct ieee80211_local *local,
                               struct sk_buff *skb);
 
-- 
1.7.6

Re: [PATCH 0/2] QoS headers for mesh

[Johannes Berg]

On Tue, 2011-09-06 at 15:26 -0700, Javier Cardona wrote:
> Mesh frames are required to have QoS headers to indicate the presence of a Mesh
> Control Header in the payload.  These patches add QoS headers to mesh frames,
> but note that they don't implement full QoS support: mesh stations don't
> currently advertise QoS capabilities.

Uh, so does mesh want full QoS support or just QoS headers? The latter
seems a little odd to me. But if it wants QoS how about zd1211rw? I
don't think that even supports QoS?

That's one thing that bothers me a little bit about this patchset --
previously, QoS frames could only happen when the device actually
supported 4 queues, now this rule seems to be broken. I don't expect
this to be a major issue, but it certainly is unexpected and will
probably be forgotten by a lot of people in the future ...

johannes

Re: [PATCH 0/2] QoS headers for mesh

[Johannes Berg]

On Wed, 2011-09-07 at 20:08 +0200, Johannes Berg wrote:
> On Tue, 2011-09-06 at 15:26 -0700, Javier Cardona wrote:
> > Mesh frames are required to have QoS headers to indicate the presence of a Mesh
> > Control Header in the payload.  These patches add QoS headers to mesh frames,
> > but note that they don't implement full QoS support: mesh stations don't
> > currently advertise QoS capabilities.
> 
> Uh, so does mesh want full QoS support or just QoS headers? The latter
> seems a little odd to me. But if it wants QoS how about zd1211rw? I
> don't think that even supports QoS?
> 
> That's one thing that bothers me a little bit about this patchset --
> previously, QoS frames could only happen when the device actually
> supported 4 queues, now this rule seems to be broken. I don't expect
> this to be a major issue, but it certainly is unexpected and will
> probably be forgotten by a lot of people in the future ...

So after looking at this again -- what I'd much rather see instead of
the second patch, and what will also require fewer changes, is *only*
doing the change to ieee80211_set_qos_hdr(). Then, if the peer is a QoS
station, we'll get the right header -- if it isn't then we can't really
mesh with it but we'll accept that for legacy reasons (for now).

I'd rather not send QoS frames to mesh stations that don't advertise QoS
capability, and I'd also rather not have to worry about sending QoS
frames when we don't actually support it locally.

johannes

Re: [PATCH 0/2] QoS headers for mesh

[Javier Cardona]

On Wed, Sep 7, 2011 at 11:08 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2011-09-06 at 15:26 -0700, Javier Cardona wrote:
>> Mesh frames are required to have QoS headers to indicate the presence of a Mesh
>> Control Header in the payload.  These patches add QoS headers to mesh frames,
>> but note that they don't implement full QoS support: mesh stations don't
>> currently advertise QoS capabilities.
>
> Uh, so does mesh want full QoS support or just QoS headers? The latter
> seems a little odd to me. But if it wants QoS how about zd1211rw? I
> don't think that even supports QoS?
>
> That's one thing that bothers me a little bit about this patchset --
> previously, QoS frames could only happen when the device actually
> supported 4 queues, now this rule seems to be broken. I don't expect
> this to be a major issue, but it certainly is unexpected and will
> probably be forgotten by a lot of people in the future ...

Some background:  The 11s task group needed a way to indicate the
presence of the mesh control header but there were no more bits
available in the frame control header.  Some earlier draft versions
stated something like "the mesh control header is present in all data
frames between mesh stations" which would require analyzing peering
frames in order to correctly parse mesh frames.  The current draft
(which will become a standard this Friday) calls for using a bit in
the QoS header.  Not pretty but definitely better than the previous
alternative.

What is unclear to me from the current spec is whether it is mandatory
that mesh stations support QoS.  If it is, we would also only support
mesh if the device supported 4 queues. I'll try to clarify this point.

Thanks,

Javier

Re: [PATCH 0/2] QoS headers for mesh

[Javier Cardona]

On Wed, Sep 7, 2011 at 11:14 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2011-09-07 at 20:08 +0200, Johannes Berg wrote:
>> On Tue, 2011-09-06 at 15:26 -0700, Javier Cardona wrote:
>> > Mesh frames are required to have QoS headers to indicate the presence of a Mesh
>> > Control Header in the payload.  These patches add QoS headers to mesh frames,
>> > but note that they don't implement full QoS support: mesh stations don't
>> > currently advertise QoS capabilities.
>>
>> Uh, so does mesh want full QoS support or just QoS headers? The latter
>> seems a little odd to me. But if it wants QoS how about zd1211rw? I
>> don't think that even supports QoS?
>>
>> That's one thing that bothers me a little bit about this patchset --
>> previously, QoS frames could only happen when the device actually
>> supported 4 queues, now this rule seems to be broken. I don't expect
>> this to be a major issue, but it certainly is unexpected and will
>> probably be forgotten by a lot of people in the future ...
>
> So after looking at this again -- what I'd much rather see instead of
> the second patch, and what will also require fewer changes, is *only*
> doing the change to ieee80211_set_qos_hdr(). Then, if the peer is a QoS
> station, we'll get the right header -- if it isn't then we can't really
> mesh with it but we'll accept that for legacy reasons (for now).
>
> I'd rather not send QoS frames to mesh stations that don't advertise QoS
> capability, and I'd also rather not have to worry about sending QoS
> frames when we don't actually support it locally.

OK.  If QoS support is mandatory for mesh stations, then definitely
that's the way to go.  We'll have to investigate how to advertise QoS
for mesh interfaces:  this is currently not happening even though all
the hardware we use for mesh supports it.
Also, we've never attempted to maintain backward compatibility with
earlier versions of the draft.

Thanks!

Javier



-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH 0/2] QoS headers for mesh

[Johannes Berg]

On Wed, 2011-09-07 at 11:36 -0700, Javier Cardona wrote:

> > So after looking at this again -- what I'd much rather see instead of
> > the second patch, and what will also require fewer changes, is *only*
> > doing the change to ieee80211_set_qos_hdr(). Then, if the peer is a QoS
> > station, we'll get the right header -- if it isn't then we can't really
> > mesh with it but we'll accept that for legacy reasons (for now).
> >
> > I'd rather not send QoS frames to mesh stations that don't advertise QoS
> > capability, and I'd also rather not have to worry about sending QoS
> > frames when we don't actually support it locally.
> 
> OK.  If QoS support is mandatory for mesh stations, then definitely
> that's the way to go.  We'll have to investigate how to advertise QoS
> for mesh interfaces:  this is currently not happening even though all
> the hardware we use for mesh supports it.

Right. I was thinking ... we could also just pretend all mesh stations
are QoS capable and always set the WLAN_STA_WME flag for them. That'd
still be cleaner in terms of how the current code flow works that adds
the QoS header and also wouldn't require any changes other than the one
to ieee80211_set_qos_hdr() (and the one to set WLAN_STA_WME of course).

johannes

Re: [PATCH 0/2] QoS headers for mesh

[Javier Cardona]

On Wed, Sep 7, 2011 at 11:36 AM, Javier Cardona <javier@cozybit.com> wrote:
> On Wed, Sep 7, 2011 at 11:14 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Wed, 2011-09-07 at 20:08 +0200, Johannes Berg wrote:
>>> On Tue, 2011-09-06 at 15:26 -0700, Javier Cardona wrote:
>>>(...)
>>> Uh, so does mesh want full QoS support or just QoS headers? The latter
>>> seems a little odd to me. But if it wants QoS how about zd1211rw? I
>>> don't think that even supports QoS?
>>> (...)
>> I'd rather not send QoS frames to mesh stations that don't advertise QoS
>> capability, and I'd also rather not have to worry about sending QoS
>> frames when we don't actually support it locally.
>
> OK.  If QoS support is mandatory for mesh stations, then definitely
> that's the way to go.  We'll have to investigate how to advertise QoS
> for mesh interfaces:  this is currently not happening even though all
> the hardware we use for mesh supports it.

Kazuyuki Sakoda, the current technical editor of TGs kindly pointed me
to the right section in the draft:  Sec 5.2.14.3 states that "mesh
STAs are QoS STAs (...) [that] implement a subset of QoS
functionality".  The use of the QoS frame format and EDCA support are
mandatory.

Based on that I'd like to prevent the creation of mesh interfaces on
phy's that don't support multiple queues.  Would you like to suggest a
good place to perform that check?

Thanks!

Javier

Re: [PATCH 0/2] QoS headers for mesh

[Johannes Berg]

On Wed, 2011-09-07 at 20:34 -0700, Javier Cardona wrote:

> Kazuyuki Sakoda, the current technical editor of TGs kindly pointed me
> to the right section in the draft:  Sec 5.2.14.3 states that "mesh
> STAs are QoS STAs (...) [that] implement a subset of QoS
> functionality".  The use of the QoS frame format and EDCA support are
> mandatory.

Makes sense.

> Based on that I'd like to prevent the creation of mesh interfaces on
> phy's that don't support multiple queues.  Would you like to suggest a
> good place to perform that check?

Since we probably can't do that in cfg80211, the most logical place
would be ieee80211_check_concurrent_iface() even though on the face of
it that function does something else; however, note the comment on
ieee80211_do_open(). Maybe it warrants a rename of the function,
something like ieee80211_check_iface_possible() maybe?

johannes