[PATCH] d80211: remove hosttime from ieee80211_rx_status

[PATCH] d80211: remove hosttime from ieee80211_rx_status

[Michael Wu]

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

d80211: remove hosttime from ieee80211_rx_status

Nobody fills hosttime in ieee80211_rx_status. Removing it allows
ieee80211_rx_status to fit in skb->cb.

Signed-off-by: Michael Wu <flamingice@sourmilk.net>
---

 include/net/d80211.h   |    1 -
 net/d80211/ieee80211.c |   25 +++++--------------------
 2 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/include/net/d80211.h b/include/net/d80211.h
index 326def5..0b7b963 100644
--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -225,7 +225,6 @@ struct ieee80211_tx_control {
  * (the subset supported by hardware) to the 802.11 code with each received
  * frame. */
 struct ieee80211_rx_status {
-        u64 hosttime;
 	u64 mactime;
         int freq; /* receive frequency in Mhz */
         int channel;
diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index bbcefa9..7a92bfe 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -2627,7 +2627,7 @@ ieee80211_fill_frame_info(struct ieee802
                 struct timespec ts;
 		struct ieee80211_rate *rate;
 
-                jiffies_to_timespec(status->hosttime, &ts);
+		jiffies_to_timespec(jiffies, &ts);
 		fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 1000000 +
 					   ts.tv_nsec / 1000);
 		fi->mactime = cpu_to_be64(status->mactime);
@@ -4019,25 +4019,11 @@ static void ieee80211_stat_refresh(unsig
 void ieee80211_rx_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb,
 			  struct ieee80211_rx_status *status)
 {
-	struct ieee80211_rx_status *saved;
 	struct ieee80211_local *local = hw_to_local(hw);
 
 	skb->dev = local->mdev;
-	saved = kmalloc(sizeof(struct ieee80211_rx_status), GFP_ATOMIC);
-	if (unlikely(!saved)) {
-		if (net_ratelimit())
-			printk(KERN_WARNING "%s: Not enough memory, "
-			       "dropping packet", skb->dev->name);
-		/* should be dev_kfree_skb_irq, but due to this function being
-		 * named _irqsafe instead of just _irq we can't be sure that
-		 * people won't call it from non-irq contexts */
-		dev_kfree_skb_any(skb);
-		return;
-	}
-	memcpy(saved, status, sizeof(struct ieee80211_rx_status));
-	/* copy pointer to saved status into skb->cb for use by tasklet */
-	memcpy(skb->cb, &saved, sizeof(saved));
-
+	/* copy status into skb->cb for use by tasklet */
+	memcpy(skb->cb, status, sizeof(struct ieee80211_rx_status));
 	skb->pkt_type = ieee80211_rx_msg;
 	skb_queue_tail(&local->skb_queue, skb);
 	tasklet_schedule(&local->tasklet);
@@ -4096,13 +4082,12 @@ static void ieee80211_tasklet_handler(un
 	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
                 switch (skb->pkt_type) {
 		case ieee80211_rx_msg:
-			/* get pointer to saved status out of skb->cb */
-			memcpy(&rx_status, skb->cb, sizeof(rx_status));
+			/* status is in skb->cb */
+			rx_status = (struct ieee80211_rx_status *) skb->cb;
 			/* Clear skb->type in order to not confuse kernel
 			 * netstack. */
 			skb->pkt_type = 0;
 			__ieee80211_rx(local_to_hw(local), skb, rx_status);
-			kfree(rx_status);
 			break;
 		case ieee80211_tx_status_msg:
 			/* get pointer to saved status out of skb->cb */

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Michael Buesch]

On Friday 09 February 2007 05:59, Michael Wu wrote:
> d80211: remove hosttime from ieee80211_rx_status
> 
> Nobody fills hosttime in ieee80211_rx_status. Removing it allows
> ieee80211_rx_status to fit in skb->cb.
> 
> Signed-off-by: Michael Wu <flamingice@sourmilk.net>
> ---

I'd like to see some kind of
BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb));
somewhere in the code to prevent unintentional future bugs.

>  include/net/d80211.h   |    1 -
>  net/d80211/ieee80211.c |   25 +++++--------------------
>  2 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/d80211.h b/include/net/d80211.h
> index 326def5..0b7b963 100644
> --- a/include/net/d80211.h
> +++ b/include/net/d80211.h
> @@ -225,7 +225,6 @@ struct ieee80211_tx_control {
>   * (the subset supported by hardware) to the 802.11 code with each received
>   * frame. */
>  struct ieee80211_rx_status {
> -        u64 hosttime;
>  	u64 mactime;
>          int freq; /* receive frequency in Mhz */
>          int channel;
> diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
> index bbcefa9..7a92bfe 100644
> --- a/net/d80211/ieee80211.c
> +++ b/net/d80211/ieee80211.c
> @@ -2627,7 +2627,7 @@ ieee80211_fill_frame_info(struct ieee802
>                  struct timespec ts;
>  		struct ieee80211_rate *rate;
>  
> -                jiffies_to_timespec(status->hosttime, &ts);
> +		jiffies_to_timespec(jiffies, &ts);
>  		fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 1000000 +
>  					   ts.tv_nsec / 1000);
>  		fi->mactime = cpu_to_be64(status->mactime);
> @@ -4019,25 +4019,11 @@ static void ieee80211_stat_refresh(unsig
>  void ieee80211_rx_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb,
>  			  struct ieee80211_rx_status *status)
>  {
> -	struct ieee80211_rx_status *saved;
>  	struct ieee80211_local *local = hw_to_local(hw);
>  
>  	skb->dev = local->mdev;
> -	saved = kmalloc(sizeof(struct ieee80211_rx_status), GFP_ATOMIC);
> -	if (unlikely(!saved)) {
> -		if (net_ratelimit())
> -			printk(KERN_WARNING "%s: Not enough memory, "
> -			       "dropping packet", skb->dev->name);
> -		/* should be dev_kfree_skb_irq, but due to this function being
> -		 * named _irqsafe instead of just _irq we can't be sure that
> -		 * people won't call it from non-irq contexts */
> -		dev_kfree_skb_any(skb);
> -		return;
> -	}
> -	memcpy(saved, status, sizeof(struct ieee80211_rx_status));
> -	/* copy pointer to saved status into skb->cb for use by tasklet */
> -	memcpy(skb->cb, &saved, sizeof(saved));
> -
> +	/* copy status into skb->cb for use by tasklet */
> +	memcpy(skb->cb, status, sizeof(struct ieee80211_rx_status));
>  	skb->pkt_type = ieee80211_rx_msg;
>  	skb_queue_tail(&local->skb_queue, skb);
>  	tasklet_schedule(&local->tasklet);
> @@ -4096,13 +4082,12 @@ static void ieee80211_tasklet_handler(un
>  	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
>                  switch (skb->pkt_type) {
>  		case ieee80211_rx_msg:
> -			/* get pointer to saved status out of skb->cb */
> -			memcpy(&rx_status, skb->cb, sizeof(rx_status));
> +			/* status is in skb->cb */
> +			rx_status = (struct ieee80211_rx_status *) skb->cb;
>  			/* Clear skb->type in order to not confuse kernel
>  			 * netstack. */
>  			skb->pkt_type = 0;
>  			__ieee80211_rx(local_to_hw(local), skb, rx_status);
> -			kfree(rx_status);
>  			break;
>  		case ieee80211_tx_status_msg:
>  			/* get pointer to saved status out of skb->cb */
> 

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Michael Wu]

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

On Friday 09 February 2007 00:50, Michael Buesch wrote:
> I'd like to see some kind of
> BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb));
> somewhere in the code to prevent unintentional future bugs.
>
Done.

--
d80211: remove hosttime from ieee80211_rx_status

Nobody fills hosttime in ieee80211_rx_status. Removing it allows
ieee80211_rx_status to fit in skb->cb.

Signed-off-by: Michael Wu <flamingice@sourmilk.net>
---

 include/net/d80211.h   |    1 -
 net/d80211/ieee80211.c |   27 +++++++--------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/include/net/d80211.h b/include/net/d80211.h
index 326def5..0b7b963 100644
--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -225,7 +225,6 @@ struct ieee80211_tx_control {
  * (the subset supported by hardware) to the 802.11 code with each received
  * frame. */
 struct ieee80211_rx_status {
-        u64 hosttime;
 	u64 mactime;
         int freq; /* receive frequency in Mhz */
         int channel;
diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index bbcefa9..c9978e2 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -2627,7 +2627,7 @@ ieee80211_fill_frame_info(struct ieee802
                 struct timespec ts;
 		struct ieee80211_rate *rate;
 
-                jiffies_to_timespec(status->hosttime, &ts);
+		jiffies_to_timespec(jiffies, &ts);
 		fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 1000000 +
 					   ts.tv_nsec / 1000);
 		fi->mactime = cpu_to_be64(status->mactime);
@@ -4019,25 +4019,13 @@ static void ieee80211_stat_refresh(unsig
 void ieee80211_rx_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb,
 			  struct ieee80211_rx_status *status)
 {
-	struct ieee80211_rx_status *saved;
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	skb->dev = local->mdev;
-	saved = kmalloc(sizeof(struct ieee80211_rx_status), GFP_ATOMIC);
-	if (unlikely(!saved)) {
-		if (net_ratelimit())
-			printk(KERN_WARNING "%s: Not enough memory, "
-			       "dropping packet", skb->dev->name);
-		/* should be dev_kfree_skb_irq, but due to this function being
-		 * named _irqsafe instead of just _irq we can't be sure that
-		 * people won't call it from non-irq contexts */
-		dev_kfree_skb_any(skb);
-		return;
-	}
-	memcpy(saved, status, sizeof(struct ieee80211_rx_status));
-	/* copy pointer to saved status into skb->cb for use by tasklet */
-	memcpy(skb->cb, &saved, sizeof(saved));
+	BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb));
 
+	skb->dev = local->mdev;
+	/* copy status into skb->cb for use by tasklet */
+	memcpy(skb->cb, status, sizeof(*status));
 	skb->pkt_type = ieee80211_rx_msg;
 	skb_queue_tail(&local->skb_queue, skb);
 	tasklet_schedule(&local->tasklet);
@@ -4096,13 +4084,12 @@ static void ieee80211_tasklet_handler(un
 	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
                 switch (skb->pkt_type) {
 		case ieee80211_rx_msg:
-			/* get pointer to saved status out of skb->cb */
-			memcpy(&rx_status, skb->cb, sizeof(rx_status));
+			/* status is in skb->cb */
+			rx_status = (struct ieee80211_rx_status *) skb->cb;
 			/* Clear skb->type in order to not confuse kernel
 			 * netstack. */
 			skb->pkt_type = 0;
 			__ieee80211_rx(local_to_hw(local), skb, rx_status);
-			kfree(rx_status);
 			break;
 		case ieee80211_tx_status_msg:
 			/* get pointer to saved status out of skb->cb */

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Jiri Benc]

On Fri, 9 Feb 2007 10:56:42 -0500, Michael Wu wrote:
> @@ -4096,13 +4084,12 @@ static void ieee80211_tasklet_handler(un
>  	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
>                  switch (skb->pkt_type) {
>  		case ieee80211_rx_msg:
> -			/* get pointer to saved status out of skb->cb */
> -			memcpy(&rx_status, skb->cb, sizeof(rx_status));
> +			/* status is in skb->cb */
> +			rx_status = (struct ieee80211_rx_status *) skb->cb;
>  			/* Clear skb->type in order to not confuse kernel
>  			 * netstack. */
>  			skb->pkt_type = 0;
>  			__ieee80211_rx(local_to_hw(local), skb, rx_status);

Uh, sorry, I haven't noticed it before: you should copy cb to a local
rx_status structure as __ieee80211_rx is allowed to destroy the skb,
yet it still needs the rx_status.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Michael Wu]

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

On Friday 09 February 2007 12:15, Jiri Benc wrote:
> Uh, sorry, I haven't noticed it before: you should copy cb to a local
> rx_status structure as __ieee80211_rx is allowed to destroy the skb,
> yet it still needs the rx_status.
>
Fixed.

--

d80211: remove hosttime from ieee80211_rx_status

Nobody fills hosttime in ieee80211_rx_status. Removing it allows
ieee80211_rx_status to fit in skb->cb.

Signed-off-by: Michael Wu <flamingice@sourmilk.net>
---

 include/net/d80211.h   |    1 -
 net/d80211/ieee80211.c |   29 ++++++++---------------------
 2 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/include/net/d80211.h b/include/net/d80211.h
index 326def5..0b7b963 100644
--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -225,7 +225,6 @@ struct ieee80211_tx_control {
  * (the subset supported by hardware) to the 802.11 code with each received
  * frame. */
 struct ieee80211_rx_status {
-        u64 hosttime;
 	u64 mactime;
         int freq; /* receive frequency in Mhz */
         int channel;
diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index bbcefa9..a0c367c 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -2627,7 +2627,7 @@ ieee80211_fill_frame_info(struct ieee802
                 struct timespec ts;
 		struct ieee80211_rate *rate;
 
-                jiffies_to_timespec(status->hosttime, &ts);
+		jiffies_to_timespec(jiffies, &ts);
 		fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 1000000 +
 					   ts.tv_nsec / 1000);
 		fi->mactime = cpu_to_be64(status->mactime);
@@ -4019,25 +4019,13 @@ static void ieee80211_stat_refresh(unsig
 void ieee80211_rx_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb,
 			  struct ieee80211_rx_status *status)
 {
-	struct ieee80211_rx_status *saved;
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	skb->dev = local->mdev;
-	saved = kmalloc(sizeof(struct ieee80211_rx_status), GFP_ATOMIC);
-	if (unlikely(!saved)) {
-		if (net_ratelimit())
-			printk(KERN_WARNING "%s: Not enough memory, "
-			       "dropping packet", skb->dev->name);
-		/* should be dev_kfree_skb_irq, but due to this function being
-		 * named _irqsafe instead of just _irq we can't be sure that
-		 * people won't call it from non-irq contexts */
-		dev_kfree_skb_any(skb);
-		return;
-	}
-	memcpy(saved, status, sizeof(struct ieee80211_rx_status));
-	/* copy pointer to saved status into skb->cb for use by tasklet */
-	memcpy(skb->cb, &saved, sizeof(saved));
+	BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb));
 
+	skb->dev = local->mdev;
+	/* copy status into skb->cb for use by tasklet */
+	memcpy(skb->cb, status, sizeof(*status));
 	skb->pkt_type = ieee80211_rx_msg;
 	skb_queue_tail(&local->skb_queue, skb);
 	tasklet_schedule(&local->tasklet);
@@ -4089,20 +4077,19 @@ static void ieee80211_tasklet_handler(un
 {
         struct ieee80211_local *local = (struct ieee80211_local *) data;
         struct sk_buff *skb;
-	struct ieee80211_rx_status *rx_status;
+	struct ieee80211_rx_status rx_status;
 	struct ieee80211_tx_status *tx_status;
 
 	while ((skb = skb_dequeue(&local->skb_queue)) ||
 	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
                 switch (skb->pkt_type) {
 		case ieee80211_rx_msg:
-			/* get pointer to saved status out of skb->cb */
+			/* status is in skb->cb */
 			memcpy(&rx_status, skb->cb, sizeof(rx_status));
 			/* Clear skb->type in order to not confuse kernel
 			 * netstack. */
 			skb->pkt_type = 0;
-			__ieee80211_rx(local_to_hw(local), skb, rx_status);
-			kfree(rx_status);
+			__ieee80211_rx(local_to_hw(local), skb, &rx_status);
 			break;
 		case ieee80211_tx_status_msg:
 			/* get pointer to saved status out of skb->cb */


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Jiri Benc]

On Fri, 9 Feb 2007 12:47:31 -0500, Michael Wu wrote:
> Nobody fills hosttime in ieee80211_rx_status. Removing it allows
> ieee80211_rx_status to fit in skb->cb.

Applied to my tree, thanks for the patch!

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] d80211: remove hosttime from ieee80211_rx_status

[Jiri Benc]

On Thu, 8 Feb 2007 23:59:42 -0500, Michael Wu wrote:
> Nobody fills hosttime in ieee80211_rx_status. Removing it allows
> ieee80211_rx_status to fit in skb->cb.

It can lead to different times being presented in the same frame
delivered to different interfaces (like two monitor interfaces). That's
probably no issue, just wanted to point it out.

Michael Buesch's comment is valid, though.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs