[PATCH 1/7] mac80211: add helpers for frame control testing

[PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

A few general categories:

ieee80211_get_* is meant to take a struct ieee80211_hdr * and returns a
pointer to somewhere in the struct, see get_SA, get_DA, get_qos_ctl.

ieee80211_is_* is meant to test whether the frame control is of a certain
ftype - data, mgmt, or ctl

ieee80211_has_* tests if particular fctl bits are set like the protected
bit, the morefrags bit, fromds, tods.

When testing for a particular stype, applicable only to one ftype, functions
like ieee80211_{ftype}_{has|is}_foo can be added, currently only
ieee80211_data_has_qos() has been added.

The _is variant will suggest an exact stype is being tested for, the _has
variant will suggest testing for particular bits being set.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/ieee80211.h |   83 ++++++++++++++++++++++++++++++++++++---------
 1 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 9300f37..5773c62 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -98,6 +98,7 @@
 
 #define IEEE80211_MAX_SSID_LEN		32
 #define IEEE80211_MAX_MESH_ID_LEN	32
+#define IEEE80211_QOS_CTL_LEN		2
 
 struct ieee80211_hdr {
 	__le16 frame_control;
@@ -109,6 +110,66 @@ struct ieee80211_hdr {
 	u8 addr4[6];
 } __attribute__ ((packed));
 
+static inline u8 *ieee80211_get_qos_ctl(struct ieee80211_hdr *hdr,
+					unsigned int hdrlen)
+{
+	return (u8 *)hdr + hdrlen - IEEE80211_QOS_CTL_LEN;
+}
+
+static inline int ieee80211_has_protected(__le16 fc)
+{
+	return !!(fc & cpu_to_le16(IEEE80211_FCTL_PROTECTED));
+}
+
+static inline int ieee80211_has_morefrags(__le16 fc)
+{
+	return !!(fc & cpu_to_le16(IEEE80211_FCTL_MOREFRAGS));
+}
+
+static inline int ieee80211_has_tods(__le16 fc)
+{
+	return (fc & cpu_to_le16(IEEE80211_FCTL_TODS)) != 0;
+}
+
+static inline int ieee80211_has_fromds(__le16 fc)
+{
+	return (fc & cpu_to_le16(IEEE80211_FCTL_FROMDS)) != 0;
+}
+
+static inline int ieee80211_has_a4(__le16 fc)
+{
+	__le16 tmp = cpu_to_le16(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
+	return (fc & tmp) == tmp;
+}
+
+static inline int ieee80211_is_mgmt(__le16 fc)
+{
+	return (fc & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
+	       cpu_to_le16(IEEE80211_FTYPE_MGMT);
+}
+
+static inline int ieee80211_is_ctl(__le16 fc)
+{
+	return (fc & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
+	       cpu_to_le16(IEEE80211_FTYPE_CTL);
+}
+
+static inline int ieee80211_is_data(__le16 fc)
+{
+	return (fc & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
+	       cpu_to_le16(IEEE80211_FTYPE_DATA);
+}
+
+static inline int ieee80211_data_has_qos(__le16 fc)
+{
+	/*
+	 * mask with QOS_DATA rather than IEEE80211_FCTL_STYPE as we just need
+	 * to check the one bit
+	 */
+	return (fc &
+	       cpu_to_le16(IEEE80211_FCTL_FTYPE | IEEE80211_STYPE_QOS_DATA)) ==
+	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_DATA);
+}
 
 struct ieee80211s_hdr {
 	u8 flags;
@@ -564,17 +625,11 @@ enum ieee80211_back_parties {
  */
 static inline u8 *ieee80211_get_SA(struct ieee80211_hdr *hdr)
 {
-	__le16 fc = hdr->frame_control;
-	fc &= cpu_to_le16(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
-
-	switch (fc) {
-	case __constant_cpu_to_le16(IEEE80211_FCTL_FROMDS):
-		return hdr->addr3;
-	case __constant_cpu_to_le16(IEEE80211_FCTL_TODS|IEEE80211_FCTL_FROMDS):
+	if (ieee80211_has_a4(hdr->frame_control))
 		return hdr->addr4;
-	default:
-		return hdr->addr2;
-	}
+	if (ieee80211_has_fromds(hdr->frame_control))
+		return hdr->addr3;
+	return hdr->addr2;
 }
 
 /**
@@ -590,10 +645,7 @@ static inline u8 *ieee80211_get_SA(struct ieee80211_hdr *hdr)
  */
 static inline u8 *ieee80211_get_DA(struct ieee80211_hdr *hdr)
 {
-	__le16 fc = hdr->frame_control;
-	fc &= cpu_to_le16(IEEE80211_FCTL_TODS);
-
-	if (fc)
+	if (ieee80211_has_tods(hdr->frame_control))
 		return hdr->addr3;
 	else
 		return hdr->addr1;
@@ -609,8 +661,7 @@ static inline u8 *ieee80211_get_DA(struct ieee80211_hdr *hdr)
  */
 static inline int ieee80211_get_morefrag(struct ieee80211_hdr *hdr)
 {
-	__le16 fc = hdr->frame_control;
-	return !!(fc & cpu_to_le16(IEEE80211_FCTL_MOREFRAGS));
+	return ieee80211_has_morefrags(hdr->frame_control);
 }
 
 #endif /* IEEE80211_H */
-- 
1.5.6.rc1.257.gba91d

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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


> +static inline u8 *ieee80211_get_qos_ctl(struct ieee80211_hdr *hdr,

I think this is wrong, the header len will probably at some point
include more things like HT headers where present or similar, so
subtracting 2 from the header len is going to be wrong as soon as that
happens because the QoS control will stay first. I think this should do
the relevant calculation itself, ie. check for A4.

> +static inline int ieee80211_has_protected(__le16 fc)

> +static inline int ieee80211_has_morefrags(__le16 fc)

> +static inline int ieee80211_has_tods(__le16 fc)

> +static inline int ieee80211_has_fromds(__le16 fc)

> +static inline int ieee80211_has_a4(__le16 fc)

> +static inline int ieee80211_is_mgmt(__le16 fc)

> +static inline int ieee80211_is_ctl(__le16 fc)

> +static inline int ieee80211_is_data(__le16 fc)

> +static inline int ieee80211_data_has_qos(__le16 fc)

Can you add kernel-doc comments for these?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

On Mon, 2008-06-09 at 09:37 +0200, Johannes Berg wrote:
> > +static inline u8 *ieee80211_get_qos_ctl(struct ieee80211_hdr *hdr,
> 
> I think this is wrong, the header len will probably at some point
> include more things like HT headers where present or similar, so
> subtracting 2 from the header len is going to be wrong as soon as that
> happens because the QoS control will stay first. I think this should do
> the relevant calculation itself, ie. check for A4.

OK.

> 
> > +static inline int ieee80211_has_protected(__le16 fc)
> 
> > +static inline int ieee80211_has_morefrags(__le16 fc)
> 
> > +static inline int ieee80211_has_tods(__le16 fc)
> 
> > +static inline int ieee80211_has_fromds(__le16 fc)
> 
> > +static inline int ieee80211_has_a4(__le16 fc)
> 
> > +static inline int ieee80211_is_mgmt(__le16 fc)
> 
> > +static inline int ieee80211_is_ctl(__le16 fc)
> 
> > +static inline int ieee80211_is_data(__le16 fc)
> 
> > +static inline int ieee80211_data_has_qos(__le16 fc)
> 
> Can you add kernel-doc comments for these?

OK.

Harvey

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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


> +static inline int ieee80211_data_has_qos(__le16 fc)
> +{
> +	/*
> +	 * mask with QOS_DATA rather than IEEE80211_FCTL_STYPE as we just need
> +	 * to check the one bit
> +	 */
> +	return (fc &
> +	       cpu_to_le16(IEEE80211_FCTL_FTYPE | IEEE80211_STYPE_QOS_DATA)) ==
> +	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_DATA);
> +}

Shouldn't that rather be _is_qos_data?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

On Mon, 2008-06-09 at 09:41 +0200, Johannes Berg wrote:
> > +static inline int ieee80211_data_has_qos(__le16 fc)
> > +{
> > +	/*
> > +	 * mask with QOS_DATA rather than IEEE80211_FCTL_STYPE as we just need
> > +	 * to check the one bit
> > +	 */
> > +	return (fc &
> > +	       cpu_to_le16(IEEE80211_FCTL_FTYPE | IEEE80211_STYPE_QOS_DATA)) ==
> > +	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_DATA);
> > +}
> 
> Shouldn't that rather be _is_qos_data?
> 

I don't think so....I was trying to keep it so _is_ meant an exact comparison and
_has_ was checking the presence of particular bits.

As this helper only checks for the one bit being set, I chose _has_.

Harvey

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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

On Mon, 2008-06-09 at 09:31 -0700, Harvey Harrison wrote:
> On Mon, 2008-06-09 at 09:41 +0200, Johannes Berg wrote:
> > > +static inline int ieee80211_data_has_qos(__le16 fc)
> > > +{
> > > +	/*
> > > +	 * mask with QOS_DATA rather than IEEE80211_FCTL_STYPE as we just need
> > > +	 * to check the one bit
> > > +	 */
> > > +	return (fc &
> > > +	       cpu_to_le16(IEEE80211_FCTL_FTYPE | IEEE80211_STYPE_QOS_DATA)) ==
> > > +	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_DATA);
> > > +}
> > 
> > Shouldn't that rather be _is_qos_data?
> > 
> 
> I don't think so....I was trying to keep it so _is_ meant an exact comparison and
> _has_ was checking the presence of particular bits.
> 
> As this helper only checks for the one bit being set, I chose _has_.

Hmm. As far as I can tell, it checks that the frame is a QoS controlled
data frame. Hence, it explicitly checks that it _is_ a data frame and
_has_ the QoS bit, but in 802.11 lingo I think that means it _is_ a
"data+qos" frame.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

On Mon, 2008-06-09 at 19:21 +0200, Johannes Berg wrote:
> On Mon, 2008-06-09 at 09:31 -0700, Harvey Harrison wrote:
> > 
> > As this helper only checks for the one bit being set, I chose _has_.
> 
> Hmm. As far as I can tell, it checks that the frame is a QoS controlled
> data frame. Hence, it explicitly checks that it _is_ a data frame and
> _has_ the QoS bit, but in 802.11 lingo I think that means it _is_ a
> "data+qos" frame.
> 

Well, it's not a big deal...what name do you want and I'll change it.

Harvey

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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

On Mon, 2008-06-09 at 10:43 -0700, Harvey Harrison wrote:
> On Mon, 2008-06-09 at 19:21 +0200, Johannes Berg wrote:
> > On Mon, 2008-06-09 at 09:31 -0700, Harvey Harrison wrote:
> > > 
> > > As this helper only checks for the one bit being set, I chose _has_.
> > 
> > Hmm. As far as I can tell, it checks that the frame is a QoS controlled
> > data frame. Hence, it explicitly checks that it _is_ a data frame and
> > _has_ the QoS bit, but in 802.11 lingo I think that means it _is_ a
> > "data+qos" frame.
> > 
> 
> Well, it's not a big deal...what name do you want and I'll change it.

I'd prefer _is_qos_data because _data_has_qos makes me think I have to
ensure that the frame is a data frame before passing it in, i.e. it will
return undefined results for a management/control frame.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

On Mon, 2008-06-09 at 19:46 +0200, Johannes Berg wrote:
> On Mon, 2008-06-09 at 10:43 -0700, Harvey Harrison wrote:
> > On Mon, 2008-06-09 at 19:21 +0200, Johannes Berg wrote:
> > > On Mon, 2008-06-09 at 09:31 -0700, Harvey Harrison wrote:
> > > > 
> > > > As this helper only checks for the one bit being set, I chose _has_.
> > > 
> > > Hmm. As far as I can tell, it checks that the frame is a QoS controlled
> > > data frame. Hence, it explicitly checks that it _is_ a data frame and
> > > _has_ the QoS bit, but in 802.11 lingo I think that means it _is_ a
> > > "data+qos" frame.
> > > 
> > 
> > Well, it's not a big deal...what name do you want and I'll change it.
> 
> I'd prefer _is_qos_data because _data_has_qos makes me think I have to
> ensure that the frame is a data frame before passing it in, i.e. it will
> return undefined results for a management/control frame.

Well, the way the other functions of the same type:

ieee80211_ctl_is_ack
ieee80211_mgmt_is_beacon

check not only that the ftype is exactly ctl/mgmt and that it is of
stype ack/beacon.  I chose ieee80211_data_has_qos because it checks
the ftype _is_ data _and_ that it _has_ qos included.  I think my
naming is more consistent with this.

Do you still think it should be changed?

Cheers,

Harvey

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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


> Well, the way the other functions of the same type:
> 
> ieee80211_ctl_is_ack
> ieee80211_mgmt_is_beacon
> 
> check not only that the ftype is exactly ctl/mgmt and that it is of
> stype ack/beacon.

Hah. Well, I think they should probably be named
ieee80211_is_ctl_ack/is_mgmt_beacon then, or just shorter
_is_ack/_is_beacon (since everybody knows ack are ctl and beacon are
mgmt.)

>   I chose ieee80211_data_has_qos because it checks
> the ftype _is_ data _and_ that it _has_ qos included.  I think my
> naming is more consistent with this.
> 
> Do you still think it should be changed?

I think you're looking at the bits too much. If you look at 802.11-2007,
Table 7-1, you'll notice that all frames that have data ftype and the
"QoS bit" set in the subtype are actually data+qos frames, just with
extra functionality added onto them by the other stype bits.

Hence, I do think it should be changed, we're more interested in the
semantic meaning ("this is a data+QoS [possibly +stuff] frame") rather
than the bitwise meaning ("this is a data frame which has some QoS bit
set"). I guess the correct way would be to say "this is data frame which
has QoS information" which we'd have to express as "_is_data_has_qos" or
something, no?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Harvey Harrison]

On Mon, 2008-06-09 at 20:02 +0200, Johannes Berg wrote:
> > Well, the way the other functions of the same type:
> >=20
> > ieee80211_ctl_is_ack
> > ieee80211_mgmt_is_beacon
> >=20
> > check not only that the ftype is exactly ctl/mgmt and that it is of
> > stype ack/beacon.
>=20
> Hah. Well, I think they should probably be named
> ieee80211_is_ctl_ack/is_mgmt_beacon then, or just shorter
> _is_ack/_is_beacon (since everybody knows ack are ctl and beacon are
> mgmt.)

OK, just for clarity, could you spell out the helper names you'd like t=
o
see, and I'll adjust accordingly, at this point I think you would agree
with:

ieee80211_is_data
=EF=BB=BFieee80211_is_ctl
=EF=BB=BFieee80211_is_mgmt

ieee80211_has_protected
=EF=BB=BFieee80211_has_morefrags
ieee80211_has_tods
ieee80211_has_fromds
ieee80211_has_a4

And you'd like to see the following changes:

ieee80211_data_has_qos -> ieee80211_is_data_qos (maybe dataqos)
ieee80211_ctl_is_ack -> ieee80211_is_ack
=EF=BB=BFieee80211_ctl_is_pspoll -> ieee80211_is_pspoll
ieee80211_ctl_is_back_req -> ieee80211_is_back_req
=EF=BB=BFieee80211_mgmt_is_beacon -> ieee80211_is_beacon

I agree that does look nicer, and it just has to be clear that these
explicity check the ftype as well.

>=20
> >   I chose ieee80211_data_has_qos because it checks
> > the ftype _is_ data _and_ that it _has_ qos included.  I think my
> > naming is more consistent with this.
> >=20
> > Do you still think it should be changed?
>=20
> I think you're looking at the bits too much. If you look at 802.11-20=
07,
> Table 7-1, you'll notice that all frames that have data ftype and the
> "QoS bit" set in the subtype are actually data+qos frames, just with
> extra functionality added onto them by the other stype bits.
>=20
> Hence, I do think it should be changed, we're more interested in the
> semantic meaning ("this is a data+QoS [possibly +stuff] frame") rathe=
r
> than the bitwise meaning ("this is a data frame which has some QoS bi=
t
> set"). I guess the correct way would be to say "this is data frame wh=
ich
> has QoS information" which we'd have to express as "_is_data_has_qos"=
 or
> something, no?

Well, I agree with your semantics argument above, so this won't be nece=
ssary.
I guess I got a little caught up in the checks themselves vs the semant=
ics
it was expressing.

So choose a colour for the bikeshed and I'll get painting ;-)

Cheers,

Harvey

--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] mac80211: add helpers for frame control testing

[Johannes Berg]

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


> OK, just for clarity, could you spell out the helper names you'd like to
> see, and I'll adjust accordingly, at this point I think you would agree
> with:
> 
> ieee80211_is_data
> ieee80211_is_ctl
> ieee80211_is_mgmt
> 
> ieee80211_has_protected
> ieee80211_has_morefrags
> ieee80211_has_tods
> ieee80211_has_fromds
> ieee80211_has_a4
> 
> And you'd like to see the following changes:
> 
> ieee80211_data_has_qos -> ieee80211_is_data_qos (maybe dataqos)
> ieee80211_ctl_is_ack -> ieee80211_is_ack
> ieee80211_ctl_is_pspoll -> ieee80211_is_pspoll
> ieee80211_ctl_is_back_req -> ieee80211_is_back_req
> ieee80211_mgmt_is_beacon -> ieee80211_is_beacon
> 
> I agree that does look nicer, and it just has to be clear that these
> explicity check the ftype as well.


> So choose a colour for the bikeshed and I'll get painting ;-)

Heh. Yeah those above seem fine to me. Arguably, "has_protected" sounds
a bit weird, but I'm not going to argue that, let's just do it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]