[RFC-PATCH] mac80211: add helpers for frame control tests

[RFC-PATCH] mac80211: add helpers for frame control tests

[Harvey Harrison]

Avoid always byteswapping the hdr->frame_control value and testing
against that.  Move the byteswapping into the constants and test
directly against the __le16 value.

One function in wpa.c moved to use some of the helpers as an example
of what this transition will look like eliminating the local var fc.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
More helpers could (should) be added to this, I just wanted to get some
feedback regarding the approach before going through the drivers and
mac80211 code.

drivers/net/wireless/iwl/iwl-helpers.h has a lot of similar helpers, but
takes the already byteswapped fc value rather than a struct ieee80211_hdr
as they do the byteswapping before testing.  There are lots of places this
could be avoided with a set of helpers like this.

 include/linux/ieee80211.h |   41 +++++++++++++++++++++++++++++++++++++++++
 net/mac80211/wpa.c        |   18 +++++++-----------
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 0b5e03e..2382cd0 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -81,6 +81,47 @@
 #define IEEE80211_STYPE_QOS_CFPOLL		0x00E0
 #define IEEE80211_STYPE_QOS_CFACKPOLL		0x00F0
 
+static inline int ieee80211_fctl_tods(struct ieee80211_hdr *hdr)
+{
+	return hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_TODS);
+}
+
+static inline int ieee80211_fctl_fromds(struct ieee80211_hdr *hdr)
+{
+	return hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FROMDS);
+}
+
+static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
+{
+	__le16 tmp = cpu_to_le16(IEEE80211_FCTL_TODS | IEEE80211_FCTL_TODS);
+	return (hdr->frame_control & tmp) == tmp;
+}
+
+static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)
+{
+	return (hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
+	       cpu_to_le16(ftype);
+}
+
+static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
+{
+	return (hdr->frame_control & cpu_to_le16(stype)) != 0;
+}
+
+static inline int ieee80211_ftype_mgmt(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_MGMT);
+}
+
+static inline int ieee80211_ftype_ctl(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_CTL);
+}
+
+static inline int ieee80211_ftype_data(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_DATA);
+}
 
 /* miscellaneous IEEE 802.11 constants */
 #define IEEE80211_MAX_FRAG_THRESHOLD	2352
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 45709ad..c37cc76 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -24,23 +24,21 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
 {
 	struct ieee80211_hdr *hdr;
 	size_t hdrlen;
-	u16 fc;
 	int a4_included;
 	u8 *pos;
 
 	hdr = (struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
 
 	hdrlen = 24;
-	if ((fc & (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) ==
-	    (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) {
+	if (ieee80211_fctl_has_a4(hdr)) {
 		hdrlen += ETH_ALEN;
+		a4_included = 1;
 		*sa = hdr->addr4;
 		*da = hdr->addr3;
-	} else if (fc & IEEE80211_FCTL_FROMDS) {
+	} else if (ieee80211_fctl_fromds(hdr)) {
 		*sa = hdr->addr3;
 		*da = hdr->addr1;
-	} else if (fc & IEEE80211_FCTL_TODS) {
+	} else if (ieee80211_fctl_tods(hdr)) {
 		*sa = hdr->addr2;
 		*da = hdr->addr3;
 	} else {
@@ -48,16 +46,14 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
 		*da = hdr->addr1;
 	}
 
-	if (fc & 0x80)
+	if (ieee80211_stype(hdr, IEEE_STYPE_QOS_DATA))
 		hdrlen += 2;
 
 	*data = skb->data + hdrlen;
 	*data_len = skb->len - hdrlen;
 
-	a4_included = (fc & (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS)) ==
-		(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
-	if ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
-	    fc & IEEE80211_STYPE_QOS_DATA) {
+	if (ieee80211_ftype_data(hdr) &&
+	    ieee80211_stype(hdr, IEEE_STYPE_QOS_DATA)) {
 		pos = (u8 *) &hdr->addr4;
 		if (a4_included)
 			pos += 6;
-- 
1.5.5.1.570.g26b5e

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Harvey Harrison]

Avoid always byteswapping the hdr->frame_control value and testing
against that.  Move the byteswapping into the constants and test
directly against the __le16 value.

One function in wpa.c moved to use some of the helpers as an example
of what this transition will look like eliminating the local var fc.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Sorry, sent the wrong one in my previous e-mail.

Harvey

 include/linux/ieee80211.h |   60 ++++++++++++++++++++++++++++++++++++++------
 net/mac80211/wpa.c        |   18 +++++--------
 2 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 0b5e03e..5212cf9 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -81,6 +81,57 @@
 #define IEEE80211_STYPE_QOS_CFPOLL		0x00E0
 #define IEEE80211_STYPE_QOS_CFACKPOLL		0x00F0
 
+struct ieee80211_hdr {
+	__le16 frame_control;
+	__le16 duration_id;
+	u8 addr1[6];
+	u8 addr2[6];
+	u8 addr3[6];
+	__le16 seq_ctrl;
+	u8 addr4[6];
+} __attribute__ ((packed));
+
+static inline int ieee80211_fctl_tods(struct ieee80211_hdr *hdr)
+{
+	return (hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_TODS)) > 0;
+}
+
+static inline int ieee80211_fctl_fromds(struct ieee80211_hdr *hdr)
+{
+	return (hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FROMDS)) > 0;
+}
+
+static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
+{
+	__le16 tmp = cpu_to_le16(IEEE80211_FCTL_TODS | IEEE80211_FCTL_TODS);
+	return (hdr->frame_control & tmp) == tmp;
+}
+
+static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)
+{
+	return (hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
+	       cpu_to_le16(ftype);
+}
+
+static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
+{
+	return (hdr->frame_control & cpu_to_le16(stype)) != 0;
+}
+
+static inline int ieee80211_ftype_mgmt(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_MGMT);
+}
+
+static inline int ieee80211_ftype_ctl(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_CTL);
+}
+
+static inline int ieee80211_ftype_data(struct ieee80211_hdr *hdr)
+{
+	return ieee80211_ftype(hdr, IEEE80211_FTYPE_DATA);
+}
 
 /* miscellaneous IEEE 802.11 constants */
 #define IEEE80211_MAX_FRAG_THRESHOLD	2352
@@ -99,15 +150,6 @@
 #define IEEE80211_MAX_SSID_LEN		32
 #define IEEE80211_MAX_MESH_ID_LEN	32
 
-struct ieee80211_hdr {
-	__le16 frame_control;
-	__le16 duration_id;
-	u8 addr1[6];
-	u8 addr2[6];
-	u8 addr3[6];
-	__le16 seq_ctrl;
-	u8 addr4[6];
-} __attribute__ ((packed));
 
 
 struct ieee80211s_hdr {
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 45709ad..f899a06 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -24,23 +24,21 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
 {
 	struct ieee80211_hdr *hdr;
 	size_t hdrlen;
-	u16 fc;
 	int a4_included;
 	u8 *pos;
 
 	hdr = (struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
 
 	hdrlen = 24;
-	if ((fc & (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) ==
-	    (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) {
+	if (ieee80211_fctl_has_a4(hdr)) {
 		hdrlen += ETH_ALEN;
+		a4_included = 1;
 		*sa = hdr->addr4;
 		*da = hdr->addr3;
-	} else if (fc & IEEE80211_FCTL_FROMDS) {
+	} else if (ieee80211_fctl_fromds(hdr)) {
 		*sa = hdr->addr3;
 		*da = hdr->addr1;
-	} else if (fc & IEEE80211_FCTL_TODS) {
+	} else if (ieee80211_fctl_tods(hdr)) {
 		*sa = hdr->addr2;
 		*da = hdr->addr3;
 	} else {
@@ -48,16 +46,14 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
 		*da = hdr->addr1;
 	}
 
-	if (fc & 0x80)
+	if (ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA))
 		hdrlen += 2;
 
 	*data = skb->data + hdrlen;
 	*data_len = skb->len - hdrlen;
 
-	a4_included = (fc & (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS)) ==
-		(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
-	if ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
-	    fc & IEEE80211_STYPE_QOS_DATA) {
+	if (ieee80211_ftype_data(hdr) &&
+	    ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)) {
 		pos = (u8 *) &hdr->addr4;
 		if (a4_included)
 			pos += 6;
-- 
1.5.5.1.570.g26b5e

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Johannes Berg]

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


> +static inline int ieee80211_fctl_tods(struct ieee80211_hdr *hdr)

> +static inline int ieee80211_fctl_fromds(struct ieee80211_hdr *hdr)

These seem fine though I don't see why you implement them with a > 0
rather than != 0 comparison?

> +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)

That seems fine too.

> +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)

That, I think, is misnamed, it should be ieee80211_is_ftype()

> +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
> +{
> +	return (hdr->frame_control & cpu_to_le16(stype)) != 0;
> +}

And that even seems implemented wrongly? stype is a 4-bit field, this
doesn't make much sense to me.

> +static inline int ieee80211_ftype_mgmt(struct ieee80211_hdr *hdr)

> +static inline int ieee80211_ftype_ctl(struct ieee80211_hdr *hdr)

> +static inline int ieee80211_ftype_data(struct ieee80211_hdr *hdr)

similarly, ieee80211_is_data() (remove the ftype, everybody hacking
wireless should know that data/mgmt/ctl are frame types)

johannes

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

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Johannes Berg]

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


> > +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
> > +{
> > +	return (hdr->frame_control & cpu_to_le16(stype)) != 0;
> > +}
> 
> And that even seems implemented wrongly? stype is a 4-bit field, this
> doesn't make much sense to me.

Now, before you get any ideas, this code


-       a4_included = (fc & (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS)) ==
-               (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
-       if ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
-           fc & IEEE80211_STYPE_QOS_DATA) {

is still correct. You really want to read the frame type bit assignments
in 802.11-2007 :)

johannes

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

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Michael Buesch]

On Friday 16 May 2008 21:38:23 Harvey Harrison wrote:
> diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
> index 45709ad..f899a06 100644
> --- a/net/mac80211/wpa.c
> +++ b/net/mac80211/wpa.c
> @@ -24,23 +24,21 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
>  {
>  	struct ieee80211_hdr *hdr;
>  	size_t hdrlen;
> -	u16 fc;
>  	int a4_included;
>  	u8 *pos;
>  
>  	hdr = (struct ieee80211_hdr *) skb->data;
> -	fc = le16_to_cpu(hdr->frame_control);
>  
>  	hdrlen = 24;
> -	if ((fc & (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) ==
> -	    (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS)) {
> +	if (ieee80211_fctl_has_a4(hdr)) {
>  		hdrlen += ETH_ALEN;
> +		a4_included = 1;
>  		*sa = hdr->addr4;
>  		*da = hdr->addr3;
> -	} else if (fc & IEEE80211_FCTL_FROMDS) {
> +	} else if (ieee80211_fctl_fromds(hdr)) {
>  		*sa = hdr->addr3;
>  		*da = hdr->addr1;
> -	} else if (fc & IEEE80211_FCTL_TODS) {
> +	} else if (ieee80211_fctl_tods(hdr)) {
>  		*sa = hdr->addr2;
>  		*da = hdr->addr3;
>  	} else {
> @@ -48,16 +46,14 @@ static int ieee80211_get_hdr_info(const struct sk_buff *skb, u8 **sa, u8 **da,
>  		*da = hdr->addr1;
>  	}
>  
> -	if (fc & 0x80)
> +	if (ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA))
>  		hdrlen += 2;
>  
>  	*data = skb->data + hdrlen;
>  	*data_len = skb->len - hdrlen;
>  
> -	a4_included = (fc & (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS)) ==
^^^^^^^^^^^^^^^^^^^
> -		(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS);
> -	if ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
> -	    fc & IEEE80211_STYPE_QOS_DATA) {
> +	if (ieee80211_ftype_data(hdr) &&
> +	    ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)) {
>  		pos = (u8 *) &hdr->addr4;
>  		if (a4_included)

Are we going to use a4_included uninitialized now?

>  			pos += 6;



-- 
Greetings Michael.

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Harvey Harrison]

On Fri, 2008-05-16 at 22:31 +0200, Johannes Berg wrote:
> > +static inline int ieee80211_fctl_tods(struct ieee80211_hdr *hdr)
>=20
> > +static inline int ieee80211_fctl_fromds(struct ieee80211_hdr *hdr)
>=20
> These seem fine though I don't see why you implement them with a > 0
> rather than !=3D 0 comparison?

No reason, I'll move to all !=3D 0

>=20
> > +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
>=20
> That seems fine too.
>=20
> > +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 f=
type)
>=20
> That, I think, is misnamed, it should be ieee80211_is_ftype()

Well, I didn't expect this to be used directly that much, hence the
following three helpers...ftype_mgmt/ctl/data

>=20
> > +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 s=
type)
> > +{
> > +	return (hdr->frame_control & cpu_to_le16(stype)) !=3D 0;
> > +}
>=20
> And that even seems implemented wrongly? stype is a 4-bit field, this
> doesn't make much sense to me.

It was meant to be used to replace code like:

=EF=BB=BFfc & IEEE80211_STYPE_QOS_DATA

=EF=BB=BFieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)

As most users of the stype bits test against a specific constant,
otherwise you could add a bunch of helpers, one for each stype
constant (similar to the ftype helpers I added.  There were only
3 in the ftype case, so I just added them all and dropped the second
parameter, in the stype case, there are more options, so I just
left one two-parameter helper.

>=20
> > +static inline int ieee80211_ftype_mgmt(struct ieee80211_hdr *hdr)
>=20
> > +static inline int ieee80211_ftype_ctl(struct ieee80211_hdr *hdr)
>=20
> > +static inline int ieee80211_ftype_data(struct ieee80211_hdr *hdr)
>=20
> similarly, ieee80211_is_data() (remove the ftype, everybody hacking
> wireless should know that data/mgmt/ctl are frame types)

OK, I'll drop ftype from these three, but leave it in the two-param
version as it is really just a helper for these three.

What do you think if I just left the stype version as-is, subject
to the usage I showed above, which is the common case.

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: [RFC-PATCH] mac80211: add helpers for frame control tests

[Johannes Berg]

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


> > > +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
> > 
> > That seems fine too.
> > 
> > > +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)
> > 
> > That, I think, is misnamed, it should be ieee80211_is_ftype()
> 
> Well, I didn't expect this to be used directly that much, hence the
> following three helpers...ftype_mgmt/ctl/data

Right. Still, I think it should be named with _is_ in there.

> > > +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u16 stype)
> > > +{
> > > +	return (hdr->frame_control & cpu_to_le16(stype)) != 0;
> > > +}
> > 
> > And that even seems implemented wrongly? stype is a 4-bit field, this
> > doesn't make much sense to me.
> 
> It was meant to be used to replace code like:
> 
> fc & IEEE80211_STYPE_QOS_DATA
> 
> ieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)
> 
> As most users of the stype bits test against a specific constant,
> otherwise you could add a bunch of helpers, one for each stype
> constant (similar to the ftype helpers I added.  There were only
> 3 in the ftype case, so I just added them all and dropped the second
> parameter, in the stype case, there are more options, so I just
> left one two-parameter helper.

Well, yes, but that's semantically unclear because of the way these bits
are used vs. how they are called. The subtype (stype) is a four-bit
field in the frame control field which isn't actually defined to have
"flags" or bits like that in it. Look at the table in IEEE 802.11-2007,
it's at the beginning of clause 7 somewhere.

Hence, hiding this simple AND behind a function that claims to check the
subtype is confusing.

johannes

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

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Harvey Harrison]

On Fri, 2008-05-16 at 23:12 +0200, Johannes Berg wrote:
> > > > +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *=
hdr)
> > >=20
> > > That seems fine too.
> > >=20
> > > > +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u=
16 ftype)
> > >=20
> > > That, I think, is misnamed, it should be ieee80211_is_ftype()
> >=20
> > Well, I didn't expect this to be used directly that much, hence the
> > following three helpers...ftype_mgmt/ctl/data
>=20
> Right. Still, I think it should be named with _is_ in there.

Yes, I agree with you, that's what I meant to write.

>=20
> > > > +static inline int ieee80211_stype(struct ieee80211_hdr *hdr, u=
16 stype)
> > > > +{
> > > > +	return (hdr->frame_control & cpu_to_le16(stype)) !=3D 0;
> > > > +}
> > >=20
> > > And that even seems implemented wrongly? stype is a 4-bit field, =
this
> > > doesn't make much sense to me.
> >=20
> > It was meant to be used to replace code like:
> >=20
> > =EF=BB=BFfc & IEEE80211_STYPE_QOS_DATA
> >=20
> > =EF=BB=BFieee80211_stype(hdr, IEEE80211_STYPE_QOS_DATA)
> >=20
> > As most users of the stype bits test against a specific constant,
> > otherwise you could add a bunch of helpers, one for each stype
> > constant (similar to the ftype helpers I added.  There were only
> > 3 in the ftype case, so I just added them all and dropped the secon=
d
> > parameter, in the stype case, there are more options, so I just
> > left one two-parameter helper.
>=20
> Well, yes, but that's semantically unclear because of the way these b=
its
> are used vs. how they are called. The subtype (stype) is a four-bit
> field in the frame control field which isn't actually defined to have
> "flags" or bits like that in it. Look at the table in IEEE 802.11-200=
7,
> it's at the beginning of clause 7 somewhere.
>=20
> Hence, hiding this simple AND behind a function that claims to check =
the
> subtype is confusing.

Ahhh, I see what you are getting at now, how about ieee80211_stype_mask=
?

That makes the & a little clearer and has better semantic meaning.

The other option is still to call it stype_mask, but change it to
return a __le16....so tests against !=3D 0 still work and really
is just a wrapper around the byteswap of the constant and the &.

What do you think of that?

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: [RFC-PATCH] mac80211: add helpers for frame control tests

[Johannes Berg]

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

On Fri, 2008-05-16 at 14:57 -0700, Harvey Harrison wrote:
> On Fri, 2008-05-16 at 23:12 +0200, Johannes Berg wrote:
> > > > > +static inline int ieee80211_fctl_has_a4(struct ieee80211_hdr *hdr)
> > > > 
> > > > That seems fine too.
> > > > 
> > > > > +static inline int ieee80211_ftype(struct ieee80211_hdr *hdr, u16 ftype)
> > > > 
> > > > That, I think, is misnamed, it should be ieee80211_is_ftype()
> > > 
> > > Well, I didn't expect this to be used directly that much, hence the
> > > following three helpers...ftype_mgmt/ctl/data
> > 
> > Right. Still, I think it should be named with _is_ in there.
> 
> Yes, I agree with you, that's what I meant to write.

Actually, maybe it should be "has" and not "is" since it's being passed
a frame that can *have* a given frame type, and is a frame of a given
type. Dunno.

> Ahhh, I see what you are getting at now, how about ieee80211_stype_mask?
> 
> That makes the & a little clearer and has better semantic meaning.
> 
> The other option is still to call it stype_mask, but change it to
> return a __le16....so tests against != 0 still work and really
> is just a wrapper around the byteswap of the constant and the &.

Hmm. I'm not sure, how is all this really used? It seems you want

ieee80211_has_qos_stype(hdr)

and

ieee80211_has_stype(hdr, stype)

instead, no?

johannes

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

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Harvey Harrison]

On Sat, 2008-05-17 at 00:03 +0200, Johannes Berg wrote:
> On Fri, 2008-05-16 at 14:57 -0700, Harvey Harrison wrote:
> > On Fri, 2008-05-16 at 23:12 +0200, Johannes Berg wrote:
> > > 
> > > Right. Still, I think it should be named with _is_ in there.
> > 
> > Yes, I agree with you, that's what I meant to write.
> 
> Actually, maybe it should be "has" and not "is" since it's being passed
> a frame that can *have* a given frame type, and is a frame of a given
> type. Dunno.

I think _is_ works best, I'll send out a few trial patches, but this
seems to make sense in the places its used.

> 
> > Ahhh, I see what you are getting at now, how about ieee80211_stype_mask?
> > 
> > That makes the & a little clearer and has better semantic meaning.
> > 
> > The other option is still to call it stype_mask, but change it to
> > return a __le16....so tests against != 0 still work and really
> > is just a wrapper around the byteswap of the constant and the &.
> 
> Hmm. I'm not sure, how is all this really used? It seems you want
> 
> ieee80211_has_qos_stype(hdr)
> and
> 
> ieee80211_has_stype(hdr, stype)
> 
> instead, no?

Just the latter I think.  But I'll do a few patches to see how it looks
first.  If there end up being a few common ones (like qos) a few
short-hand versions might be appropriate, I'll see how it goes.

Harvey

Re: [RFC-PATCH] mac80211: add helpers for frame control tests

[Johannes Berg]

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


> > > Ahhh, I see what you are getting at now, how about ieee80211_stype_mask?
> > > 
> > > That makes the & a little clearer and has better semantic meaning.
> > > 
> > > The other option is still to call it stype_mask, but change it to
> > > return a __le16....so tests against != 0 still work and really
> > > is just a wrapper around the byteswap of the constant and the &.
> > 
> > Hmm. I'm not sure, how is all this really used? It seems you want
> > 
> > ieee80211_has_qos_stype(hdr)
> > and
> > 
> > ieee80211_has_stype(hdr, stype)
> > 
> > instead, no?
> 
> Just the latter I think.  But I'll do a few patches to see how it looks
> first.  If there end up being a few common ones (like qos) a few
> short-hand versions might be appropriate, I'll see how it goes.

Yeah but I was thinking of ieee80211_stype_is() or whatever you want to
call it (I think I actually like it that way around better than
is_stype/ftype) to check the exact match of the four bits, i.e. 
	fc & stypemask == argument

and the QoS one check that the bit is set.

johannes

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