[PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Helmut Schaa]

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 include/linux/ieee80211.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 17f2a76..18326eb 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -544,6 +544,15 @@ static inline int ieee80211_is_qos_nullfunc(__le16 fc)
 	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_NULLFUNC);
 }
 
+/**
+ * ieee80211_has_frag - check if IEEE80211_SCTL_FRAG is set
+ * @seq_ctrl: frame sequence control bytes in little-endian byteorder
+ */
+static inline int ieee80211_has_frag(__le16 seq_ctrl)
+{
+	return (seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)) != 0;
+}
+
 struct ieee80211s_hdr {
 	u8 flags;
 	u8 ttl;
-- 
1.7.7

[PATCH 2/2] mac80211: Make use of ieee80211_is_* functions in tx status path

[Helmut Schaa]

Use ieee80211_is_data, ieee80211_is_mgmt and ieee80211_has_frag in the
tx status path. This makes the code easier to read and allows us to
remove two local variables: frag and type.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/status.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 46222ce..d0f4d9a 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -340,7 +340,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	u16 frag, type;
 	__le16 fc;
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_sub_if_data *sdata;
@@ -476,12 +475,8 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	 * Fragments are passed to low-level drivers as separate skbs, so these
 	 * are actually fragments, not frames. Update frame counters only for
 	 * the first fragment of the frame. */
-
-	frag = le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG;
-	type = le16_to_cpu(hdr->frame_control) & IEEE80211_FCTL_FTYPE;
-
 	if (info->flags & IEEE80211_TX_STAT_ACK) {
-		if (frag == 0) {
+		if (!ieee80211_has_frag(hdr->seq_ctrl)) {
 			local->dot11TransmittedFrameCount++;
 			if (is_multicast_ether_addr(hdr->addr1))
 				local->dot11MulticastTransmittedFrameCount++;
@@ -496,11 +491,11 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 		 * with a multicast address in the address 1 field of type Data
 		 * or Management. */
 		if (!is_multicast_ether_addr(hdr->addr1) ||
-		    type == IEEE80211_FTYPE_DATA ||
-		    type == IEEE80211_FTYPE_MGMT)
+		    ieee80211_is_data(fc) ||
+		    ieee80211_is_mgmt(fc))
 			local->dot11TransmittedFragmentCount++;
 	} else {
-		if (frag == 0)
+		if (!ieee80211_has_frag(hdr->seq_ctrl))
 			local->dot11FailedCount++;
 	}
 
@@ -572,7 +567,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 
 	/* Need to make a copy before skb->cb gets cleared */
 	send_to_cooked = !!(info->flags & IEEE80211_TX_CTL_INJECTED) ||
-			(type != IEEE80211_FTYPE_DATA);
+			 !(ieee80211_is_data(fc));
 
 	/*
 	 * This is a bit racy but we can avoid a lot of work
-- 
1.7.7

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Johannes Berg]

On Thu, 2011-12-08 at 10:21 +0100, Helmut Schaa wrote:
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  include/linux/ieee80211.h |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
> index 17f2a76..18326eb 100644
> --- a/include/linux/ieee80211.h
> +++ b/include/linux/ieee80211.h
> @@ -544,6 +544,15 @@ static inline int ieee80211_is_qos_nullfunc(__le16 fc)
>  	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_NULLFUNC);
>  }
>  
> +/**
> + * ieee80211_has_frag - check if IEEE80211_SCTL_FRAG is set
> + * @seq_ctrl: frame sequence control bytes in little-endian byteorder
> + */
> +static inline int ieee80211_has_frag(__le16 seq_ctrl)

bool? not like I care much though

Would _is_frag() read nicer? The piece you're checking is a fragment I
think?

> +{
> +	return (seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)) != 0;
> +}
> +

Maybe you should move that close to ieee80211_has_morefrags() so it's
easy to tell the difference? :-) (that doesn't use bool either, heh)

johannes

Re: [PATCH 2/2] mac80211: Make use of ieee80211_is_* functions in tx status path

[Johannes Berg]

On Thu, 2011-12-08 at 10:21 +0100, Helmut Schaa wrote:
> Use ieee80211_is_data, ieee80211_is_mgmt and ieee80211_has_frag in the
> tx status path. This makes the code easier to read and allows us to
> remove two local variables: frag and type.

Nice, thanks :)

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  net/mac80211/status.c |   15 +++++----------
>  1 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index 46222ce..d0f4d9a 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -340,7 +340,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
>  	struct ieee80211_local *local = hw_to_local(hw);
>  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> -	u16 frag, type;
>  	__le16 fc;
>  	struct ieee80211_supported_band *sband;
>  	struct ieee80211_sub_if_data *sdata;
> @@ -476,12 +475,8 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>  	 * Fragments are passed to low-level drivers as separate skbs, so these
>  	 * are actually fragments, not frames. Update frame counters only for
>  	 * the first fragment of the frame. */
> -
> -	frag = le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG;
> -	type = le16_to_cpu(hdr->frame_control) & IEEE80211_FCTL_FTYPE;
> -
>  	if (info->flags & IEEE80211_TX_STAT_ACK) {
> -		if (frag == 0) {
> +		if (!ieee80211_has_frag(hdr->seq_ctrl)) {
>  			local->dot11TransmittedFrameCount++;
>  			if (is_multicast_ether_addr(hdr->addr1))
>  				local->dot11MulticastTransmittedFrameCount++;
> @@ -496,11 +491,11 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>  		 * with a multicast address in the address 1 field of type Data
>  		 * or Management. */
>  		if (!is_multicast_ether_addr(hdr->addr1) ||
> -		    type == IEEE80211_FTYPE_DATA ||
> -		    type == IEEE80211_FTYPE_MGMT)
> +		    ieee80211_is_data(fc) ||
> +		    ieee80211_is_mgmt(fc))
>  			local->dot11TransmittedFragmentCount++;
>  	} else {
> -		if (frag == 0)
> +		if (!ieee80211_has_frag(hdr->seq_ctrl))
>  			local->dot11FailedCount++;
>  	}
>  
> @@ -572,7 +567,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>  
>  	/* Need to make a copy before skb->cb gets cleared */
>  	send_to_cooked = !!(info->flags & IEEE80211_TX_CTL_INJECTED) ||
> -			(type != IEEE80211_FTYPE_DATA);
> +			 !(ieee80211_is_data(fc));
>  
>  	/*
>  	 * This is a bit racy but we can avoid a lot of work

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Johannes Berg]

On Thu, 2011-12-08 at 10:25 +0100, Johannes Berg wrote:
> On Thu, 2011-12-08 at 10:21 +0100, Helmut Schaa wrote:
> > Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> > ---
> >  include/linux/ieee80211.h |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
> > index 17f2a76..18326eb 100644
> > --- a/include/linux/ieee80211.h
> > +++ b/include/linux/ieee80211.h
> > @@ -544,6 +544,15 @@ static inline int ieee80211_is_qos_nullfunc(__le16 fc)
> >  	       cpu_to_le16(IEEE80211_FTYPE_DATA | IEEE80211_STYPE_QOS_NULLFUNC);
> >  }
> >  
> > +/**
> > + * ieee80211_has_frag - check if IEEE80211_SCTL_FRAG is set
> > + * @seq_ctrl: frame sequence control bytes in little-endian byteorder
> > + */
> > +static inline int ieee80211_has_frag(__le16 seq_ctrl)
> 
> bool? not like I care much though
> 
> Would _is_frag() read nicer? The piece you're checking is a fragment I
> think?

Never mind ... the first fragment has is a fragment too and doesn't
return true here.

johannes

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Helmut Schaa]

On Thu, Dec 8, 2011 at 10:25 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> +{
>> +     return (seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)) != 0;
>> +}
>> +
>
> Maybe you should move that close to ieee80211_has_morefrags() so it's
> easy to tell the difference? :-) (that doesn't use bool either, heh)

Since all ieee80211_is/has_* functions use int already I took the same
approach.

And ieee80211_has_morefrags operates on the frame control while
this operates on seq_ctrl. Hence, I placed it at the end but if you prefer
to have it closer to ieee80211_has_morefrags I can do that too :)

Helmut

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Helmut Schaa]

On Thu, Dec 8, 2011 at 10:26 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> Would _is_frag() read nicer? The piece you're checking is a fragment I
>> think?
>
> Never mind ... the first fragment has is a fragment too and doesn't
> return true here.

We could also call it ieee80211_is_first_frag and swap the logic?

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Johannes Berg]

On Thu, 2011-12-08 at 10:38 +0100, Helmut Schaa wrote:
> On Thu, Dec 8, 2011 at 10:25 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >> +{
> >> +     return (seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)) != 0;
> >> +}
> >> +
> >
> > Maybe you should move that close to ieee80211_has_morefrags() so it's
> > easy to tell the difference? :-) (that doesn't use bool either, heh)
> 
> Since all ieee80211_is/has_* functions use int already I took the same
> approach.
> 
> And ieee80211_has_morefrags operates on the frame control while
> this operates on seq_ctrl. Hence, I placed it at the end but if you prefer
> to have it closer to ieee80211_has_morefrags I can do that too :)

Oh, well, whatever, I don't really care :)

johannes

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Johannes Berg]

On Thu, 2011-12-08 at 10:41 +0100, Helmut Schaa wrote:
> On Thu, Dec 8, 2011 at 10:26 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >> Would _is_frag() read nicer? The piece you're checking is a fragment I
> >> think?
> >
> > Never mind ... the first fragment has is a fragment too and doesn't
> > return true here.
> 
> We could also call it ieee80211_is_first_frag and swap the logic?

That seems like a good idea.

johannes

Re: [PATCH 1/2] ieee80211: Introduce ieee80211_has_frags

[Helmut Schaa]

On Thu, Dec 8, 2011 at 11:39 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2011-12-08 at 10:41 +0100, Helmut Schaa wrote:
>> On Thu, Dec 8, 2011 at 10:26 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> >> Would _is_frag() read nicer? The piece you're checking is a fragment I
>> >> think?
>> >
>> > Never mind ... the first fragment has is a fragment too and doesn't
>> > return true here.
>>
>> We could also call it ieee80211_is_first_frag and swap the logic?
>
> That seems like a good idea.

John, I've just sent a v2 of this ...