2008-06-06 17:59:47

by Harvey Harrison

[permalink] [raw]
Subject: [PATCH 3/7] mac80211: add utility function to get header length

Take a __le16 directly rather than a host-endian value.

Signed-off-by: Harvey Harrison <[email protected]>
---
include/net/mac80211.h | 6 ++++++
net/mac80211/util.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 1196de8..53c3b5e 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1548,6 +1548,12 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
int ieee80211_get_hdrlen(u16 fc);

/**
+ * ieee80211_hdrlen - get header length in bytes from frame control
+ * @fc: frame control field in little-endian format
+ */
+unsigned int ieee80211_hdrlen(__le16 fc);
+
+/**
* ieee80211_get_tkip_key - get a TKIP rc4 for skb
*
* This function computes a TKIP rc4 key for an skb. It computes
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 6513bc2..fade001 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -133,6 +133,38 @@ int ieee80211_get_hdrlen(u16 fc)
}
EXPORT_SYMBOL(ieee80211_get_hdrlen);

+unsigned int ieee80211_hdrlen(__le16 fc)
+{
+ unsigned int hdrlen = 24;
+
+ if (ieee80211_is_data(fc)) {
+ if (ieee80211_has_a4(fc))
+ hdrlen = 30;
+ if (ieee80211_data_has_qos(fc))
+ hdrlen += IEEE80211_QOS_CTL_LEN;
+ goto out;
+ }
+
+ if (ieee80211_is_ctl(fc)) {
+ /*
+ * ACK and CTS are 10 bytes, all others 16. To see how
+ * to get this condition consider
+ * subtype mask: 0b0000000011110000 (0x00F0)
+ * ACK subtype: 0b0000000011010000 (0x00D0)
+ * CTS subtype: 0b0000000011000000 (0x00C0)
+ * bits that matter: ^^^ (0x00E0)
+ * value of those: 0b0000000011000000 (0x00C0)
+ */
+ if ((fc & cpu_to_le16(0x00E0)) == cpu_to_le16(0x00C0))
+ hdrlen = 10;
+ else
+ hdrlen = 16;
+ }
+out:
+ return hdrlen;
+}
+EXPORT_SYMBOL(ieee80211_hdrlen);
+
int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
{
const struct ieee80211_hdr *hdr = (const struct ieee80211_hdr *) skb->data;
--
1.5.6.rc1.257.gba91d




2008-06-09 16:24:24

by Harvey Harrison

[permalink] [raw]
Subject: Re: [PATCH 3/7] mac80211: add utility function to get header length

On Mon, 2008-06-09 at 13:01 +0300, Tomas Winkler wrote:
> On Mon, Jun 9, 2008 at 12:16 PM, Johannes Berg
> <[email protected]> wrote:
> >
> >> /**
> >> + * ieee80211_hdrlen - get header length in bytes from frame control
> >> + * @fc: frame control field in little-endian format
> >> + */
> >> +unsigned int ieee80211_hdrlen(__le16 fc);
> >> +
> >
> >> +EXPORT_SYMBOL(ieee80211_hdrlen);
> >
> > Do we really need to export that?
>
> so now we have hdrlen(u16) and hdrlen(__le16) it will be fun.
>
> So I guess will we converting idioms
> u16 fc = le16_to_cpu(hdr->frame_control);
> int hdr_len = ieee80211_get_hdrlen(fc);
> to
> int hdr_len = ieee80211_hdrlen(hdr->frame_control)
>
> This is how it used in driver code so it make sense to export this
> function and remove ieee80211_get_hdrlen(fc)

Yes, that was my thinking, I just did it this way to avoid the flag day
change, I'll trickle the changes in and then remove _get_hdrlen.

>
> Since all fc operations are bitwise 'and' and 'or'
> u16 rx->fc can be dropped in future as well

I was going to convert it to a __le16, but if it is just a copy of the
->frame_control in the header, I'll look at removing it instead.

Cheers,

Harvey


2008-06-09 17:25:55

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/7] mac80211: add utility function to get header length


> > So I guess will we converting idioms
> > u16 fc = le16_to_cpu(hdr->frame_control);
> > int hdr_len = ieee80211_get_hdrlen(fc);
> > to
> > int hdr_len = ieee80211_hdrlen(hdr->frame_control)
> >
> > This is how it used in driver code so it make sense to export this
> > function and remove ieee80211_get_hdrlen(fc)
>
> Yes, that was my thinking, I just did it this way to avoid the flag day
> change, I'll trickle the changes in and then remove _get_hdrlen.

Sounds good to me.

> > Since all fc operations are bitwise 'and' and 'or'
> > u16 rx->fc can be dropped in future as well
>
> I was going to convert it to a __le16, but if it is just a copy of the
> ->frame_control in the header, I'll look at removing it instead.

Yeah, I think rx->fc was meant to be a cpu-byteorder copy of
hdr->frame_control to avoid repeated byteswapping. If we do all
operations on the constants instead as you're doing with this series, we
ought to be able to remove it. Bonus point: that gets rid of the
possible "rx->fc is out of sync" issue we had to fix once already.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-06-09 10:01:24

by Tomas Winkler

[permalink] [raw]
Subject: Re: [PATCH 3/7] mac80211: add utility function to get header length

On Mon, Jun 9, 2008 at 12:16 PM, Johannes Berg
<[email protected]> wrote:
>
>> /**
>> + * ieee80211_hdrlen - get header length in bytes from frame control
>> + * @fc: frame control field in little-endian format
>> + */
>> +unsigned int ieee80211_hdrlen(__le16 fc);
>> +
>
>> +EXPORT_SYMBOL(ieee80211_hdrlen);
>
> Do we really need to export that?

so now we have hdrlen(u16) and hdrlen(__le16) it will be fun.

So I guess will we converting idioms
u16 fc = le16_to_cpu(hdr->frame_control);
int hdr_len = ieee80211_get_hdrlen(fc);
to
int hdr_len = ieee80211_hdrlen(hdr->frame_control)

This is how it used in driver code so it make sense to export this
function and remove ieee80211_get_hdrlen(fc)

Since all fc operations are bitwise 'and' and 'or'
u16 rx->fc can be dropped in future as well

Tomas

2008-06-09 09:16:40

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/7] mac80211: add utility function to get header length


> /**
> + * ieee80211_hdrlen - get header length in bytes from frame control
> + * @fc: frame control field in little-endian format
> + */
> +unsigned int ieee80211_hdrlen(__le16 fc);
> +

> +EXPORT_SYMBOL(ieee80211_hdrlen);

Do we really need to export that?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-06-09 17:33:55

by Harvey Harrison

[permalink] [raw]
Subject: Re: [PATCH 3/7] mac80211: add utility function to get header length

On Mon, 2008-06-09 at 19:24 +0200, Johannes Berg wrote:
> > > So I guess will we converting idioms
> > > u16 fc = le16_to_cpu(hdr->frame_control);
> > > int hdr_len = ieee80211_get_hdrlen(fc);
> > > to
> > > int hdr_len = ieee80211_hdrlen(hdr->frame_control)
> > >
> > > This is how it used in driver code so it make sense to export this
> > > function and remove ieee80211_get_hdrlen(fc)
> >
> > Yes, that was my thinking, I just did it this way to avoid the flag day
> > change, I'll trickle the changes in and then remove _get_hdrlen.
>
> Sounds good to me.
>

OK, will keep going on this then.

> > > Since all fc operations are bitwise 'and' and 'or'
> > > u16 rx->fc can be dropped in future as well
> >
> > I was going to convert it to a __le16, but if it is just a copy of the
> > ->frame_control in the header, I'll look at removing it instead.
>
> Yeah, I think rx->fc was meant to be a cpu-byteorder copy of
> hdr->frame_control to avoid repeated byteswapping. If we do all
> operations on the constants instead as you're doing with this series, we
> ought to be able to remove it. Bonus point: that gets rid of the
> possible "rx->fc is out of sync" issue we had to fix once already.

That and be arches eliminate a bunch of byteswaps.

Cheers,

Harvey