ath9k: use ieee80211_free_txskb

RFC New Christian Lamparter 2 participants 10 messages Feb 27, 2012 – Mar 6, 2012 Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

Christian Lamparter

author

2012-02-27 16:10 UTC #0 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.

Signed-off-by Christian Lamparter <[email protected]>

---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.

The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!

Regards,
Chr

---
 drivers/net/wireless/ath/ath9k/main.c |    2 +-
 drivers/net/wireless/ath/ath9k/xmit.c |    6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +1 -1

index c81304d..4471ed9 100644
@@ -1162,7 +1162,7 @@ static void ath9k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 
 	return;
 exit:
-	dev_kfree_skb_any(skb);
+	ieee80211_free_txskb(sc->hw, skb);
 }
 
 static void ath9k_stop(struct ieee80211_hw *hw)

drivers/net/wireless/ath/ath9k/xmit.c +5 -1

index 3182408..0dadbc3 100644
@@ -1858,7 +1858,11 @@ static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
 	return bf;
 
 error:
-	dev_kfree_skb_any(skb);
+
+	if (bf->bf_state.bfs_paprd)
+		dev_kfree_skb_any(skb);
+	else
+		ieee80211_free_txskb(sc->hw, skb);
 	return NULL;
 }

Signature · 1 line

1.7.9.1

Christian Lamparter

author

2012-02-27 16:32 UTC #1 lore raw

RFC 2/2 ath9k_htc: use ieee80211_free_txskb

With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.

Signed-off-by Christian Lamparter <[email protected]>

---
It compiles, but not tested.

---
 drivers/net/wireless/ath/ath9k/htc_drv_txrx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +2 -2

index 3e40a64..e958704 100644
@@ -451,7 +451,7 @@ static void ath9k_htc_tx_process(struct ath9k_htc_priv *priv,
 
 	slot = strip_drv_header(priv, skb);
 	if (slot < 0) {
-		dev_kfree_skb_any(skb);
+		ieee80211_free_txskb(priv->hw, skb);
 		return;
 	}

@@ -687,7 +687,7 @@ void ath9k_htc_txep(void *drv_priv, struct sk_buff *skb,
 
 	epid_queue = get_htc_epid_queue(priv, ep_id);
 	if (!epid_queue) {
-		dev_kfree_skb_any(skb);
+		ieee80211_free_txskb(priv->hw, skb);
 		return;
 	}

Signature · 1 line

1.7.9.1

Mohammed Shafi Shajakhan

2012-03-01 13:58 UTC #2 lore raw

RFC 2/2 ath9k_htc: use ieee80211_free_txskb

Hi Christian,

Quoted 24 lines · Christian Lamparter

With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It compiles, but not tested.
---
  drivers/net/wireless/ath/ath9k/htc_drv_txrx.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +1 -1
index 3e40a64..e958704 100644
@@ -451,7 +451,7 @@ static void ath9k_htc_tx_process(struct ath9k_htc_priv *priv,
 
  	slot = strip_drv_header(priv, skb);
  	if (slot<  0) {
-		dev_kfree_skb_any(skb);
+		ieee80211_free_txskb(priv->hw, skb);
  		return;
  	}
 

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c void ath9k_htc_txep(void *drv_priv, struct sk_buff *skb, · line 693

Quoted 8 lines

drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +1 -1

@@ -687,7 +687,7 @@ void ath9k_htc_txep(void *drv_priv, struct sk_buff *skb,
 
  	epid_queue = get_htc_epid_queue(priv, ep_id);
  	if (!epid_queue) {
-		dev_kfree_skb_any(skb);
+		ieee80211_free_txskb(priv->hw, skb);
  		return;
  	}

also we should do it ath9k_htc_tx in htc_drv_main.c, i don't know very much about ath9k_htc.

Signature · 2 lines

thanks,
shafi

Mohammed Shafi Shajakhan

2012-03-01 05:39 UTC #3 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

In reply to Christian Lamparter

Hi Christian,

Quoted 8 lines · Christian Lamparter

With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.

Quoted 5 lines

The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!

yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.

Quoted 38 lines

Regards,
Chr

---
  drivers/net/wireless/ath/ath9k/main.c |    2 +-
  drivers/net/wireless/ath/ath9k/xmit.c |    6 +++++-
  2 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +1 -1

index c81304d..4471ed9 100644
@@ -1162,7 +1162,7 @@ static void ath9k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 
  	return;
  exit:
-	dev_kfree_skb_any(skb);
+	ieee80211_free_txskb(sc->hw, skb);
  }
 
  static void ath9k_stop(struct ieee80211_hw *hw)

drivers/net/wireless/ath/ath9k/xmit.c +5 -1

index 3182408..0dadbc3 100644
@@ -1858,7 +1858,11 @@ static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
  	return bf;
 
  error:
-	dev_kfree_skb_any(skb);
+
+	if (bf->bf_state.bfs_paprd)
+		dev_kfree_skb_any(skb);
+	else
+		ieee80211_free_txskb(sc->hw, skb);
  	return NULL;
  }

Signature · 2 lines

thanks,
shafi

Mohammed Shafi Shajakhan

2012-03-01 11:42 UTC #4 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

Quoted 17 lines · Mohammed Shafi Shajakhan

Hi Christian,
Quoted 8 lines · Christian Lamparter
With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.
Quoted 5 lines
The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!

Quoted 4 lines

yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.

got some idea, need to make sure this works perfectly, paprd frames are not aggregated, so we can look only at the non-aggregated path.

lets ignore aggregated path:

*ath_tx_form_aggr
*ath_tx_send_ampdu

non-aggregated path :

*ath_tx_start_dma
*ath_send_normal

-> 'ath_tx_setup_buffer' called from 'ath_tx_start_dma' can have a 'is_paprd' check based on 'txctl->paprd' which holds which chain we are sending PAPRD frame.

-> 'ath_send_normal' called from
-ath_tx_start_dma ( then 'bf' should
be valid and we would not have called 'ath_tx_setup_buffer'
-ath_tx_flush_tid ( which is an aggregation path) ignore it.

attached rough patch for ath9k, need to be compiled and tested. i will also analyze if this can be done in a better way or any flaws in it

Quoted 44 lines

Quoted 42 lines
Regards,
Chr
---
drivers/net/wireless/ath/ath9k/main.c | 2 +-
drivers/net/wireless/ath/ath9k/xmit.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath9k/main.c +0 -0
index c81304d..4471ed9 100644
@@ -1162,7 +1162,7 @@ static void ath9k_tx(struct ieee80211_hw *hw,
 struct sk_buff *skb)
 
 return;
exit:
- dev_kfree_skb_any(skb);
+ ieee80211_free_txskb(sc->hw, skb);
}
static void ath9k_stop(struct ieee80211_hw *hw)
drivers/net/wireless/ath/ath9k/xmit.c +0 -0
index 3182408..0dadbc3 100644
@@ -1858,7 +1858,11 @@ static struct ath_buf
 *ath_tx_setup_buffer(struct ath_softc *sc,
 return bf;
 
error:
- dev_kfree_skb_any(skb);
+
+ if (bf->bf_state.bfs_paprd)
+ dev_kfree_skb_any(skb);
+ else
+ ieee80211_free_txskb(sc->hw, skb);
return NULL;
}

Signature · 9 lines

thanks,
shafi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: use-mac80211-api-freeing-skb.patch
Type: text/x-patch
Size: 2865 bytes
Desc: not available
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20120301/ddeb851f/attachment.bin

Mohammed Shafi Shajakhan

2012-03-01 13:46 UTC #5 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

Quoted 42 lines · Mohammed Shafi Shajakhan

Quoted 17 lines · Mohammed Shafi Shajakhan
Hi Christian,
Quoted 8 lines · Christian Lamparter
With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.
Quoted 5 lines
The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!
Quoted 4 lines
yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.
got some idea, need to make sure this works perfectly, paprd frames are not aggregated, so we can look only at the non-aggregated path.
lets ignore aggregated path:
*ath_tx_form_aggr
*ath_tx_send_ampdu
non-aggregated path :
*ath_tx_start_dma
*ath_send_normal
-> 'ath_tx_setup_buffer' called from 'ath_tx_start_dma' can have a 'is_paprd' check based on 'txctl->paprd' which holds which chain we are sending PAPRD frame.

Quoted 9 lines

-> 'ath_send_normal' called from
-ath_tx_start_dma ( then 'bf' should be valid and we would not have called 'ath_tx_setup_buffer'
-ath_tx_flush_tid ( which is an aggregation path) ignore it.
attached rough patch for ath9k, need to be compiled and tested. i will also analyze if this can be done in a better way or any flaws in it

compilation fails with previous patch, forgot to declare the field in the declaration, attached patch passes compilation.

Signature · 9 lines

thanks,
shafi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: use-mac80211-api-freeing-skb.patch
Type: text/x-patch
Size: 3092 bytes
Desc: not available
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20120301/21a9b702/attachment.bin

Mohammed Shafi Shajakhan

2012-03-05 15:00 UTC #6 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

Hi Christian,

ping

Quoted 49 lines · Mohammed Shafi Shajakhan

Quoted 48 lines · Mohammed Shafi Shajakhan
Quoted 17 lines · Mohammed Shafi Shajakhan
Hi Christian,
Quoted 8 lines · Christian Lamparter
With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.
Quoted 5 lines
The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!
Quoted 4 lines
yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.
got some idea, need to make sure this works perfectly, paprd frames are not aggregated, so we can look only at the non-aggregated path.
lets ignore aggregated path:
*ath_tx_form_aggr
*ath_tx_send_ampdu
non-aggregated path :
*ath_tx_start_dma
*ath_send_normal
-> 'ath_tx_setup_buffer' called from 'ath_tx_start_dma' can have a 'is_paprd' check based on 'txctl->paprd' which holds which chain we are sending PAPRD frame.
-> 'ath_send_normal' called from
-ath_tx_start_dma ( then 'bf' should be valid and we would not have called 'ath_tx_setup_buffer'
-ath_tx_flush_tid ( which is an aggregation path) ignore it.

Quoted 8 lines

Quoted 3 lines
attached rough patch for ath9k, need to be compiled and tested. i will also analyze if this can be done in a better way or any flaws in it
compilation fails with previous patch, forgot to declare the field in the declaration, attached patch passes compilation.

would you like me to test this patch, i can just ran a test parallel y enabling PAPRD,with this proposed patch.

Signature · 2 lines

thanks,
shafi

Mohammed Shafi Shajakhan

2012-03-05 15:01 UTC #7 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

In reply to Mohammed Shafi Shajakhan

Hi Christian,

ping

Quoted 49 lines · Mohammed Shafi Shajakhan

Quoted 48 lines · Mohammed Shafi Shajakhan
Quoted 17 lines · Mohammed Shafi Shajakhan
Hi Christian,
Quoted 8 lines · Christian Lamparter
With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.
Quoted 5 lines
The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!
Quoted 4 lines
yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.
got some idea, need to make sure this works perfectly, paprd frames are not aggregated, so we can look only at the non-aggregated path.
lets ignore aggregated path:
*ath_tx_form_aggr
*ath_tx_send_ampdu
non-aggregated path :
*ath_tx_start_dma
*ath_send_normal
-> 'ath_tx_setup_buffer' called from 'ath_tx_start_dma' can have a 'is_paprd' check based on 'txctl->paprd' which holds which chain we are sending PAPRD frame.
-> 'ath_send_normal' called from
-ath_tx_start_dma ( then 'bf' should be valid and we would not have called 'ath_tx_setup_buffer'
-ath_tx_flush_tid ( which is an aggregation path) ignore it.

Quoted 8 lines

Quoted 3 lines
attached rough patch for ath9k, need to be compiled and tested. i will also analyze if this can be done in a better way or any flaws in it
compilation fails with previous patch, forgot to declare the field in the declaration, attached patch passes compilation.

would you like me to test this patch, i can just ran a bidirectional traffic test parallel y enabling PAPRD,with this proposed patch.

Signature · 2 lines

thanks,
shafi

Christian Lamparter

author

2012-03-05 18:43 UTC #8 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

Quoted 1 line · Mohammed Shafi Shajakhan

ping

pong !

Quoted 59 lines · Mohammed Shafi Shajakhan

Quoted 49 lines · Mohammed Shafi Shajakhan
Quoted 48 lines · Mohammed Shafi Shajakhan
Quoted 17 lines
Hi Christian,
Quoted 8 lines · Christian Lamparter
With the new tx status API: "mac80211: implement wifi TX status" All skb originating from mac80211 needs to be given back to mac80211.
Signed-off-by Christian Lamparter<[email protected]>
---
It's high time we change all calls in the tx-path from dev_kfree_skb into ieee80211_free_txskb.
Quoted 5 lines
The call in ath9k_tx is straightforward, but the one in ath_tx_setup_buffer gives me headaches. I'm not sure if we even need to check bf->bf_state.bfs_paprd at this stage since ath_tx_start_dma sets bf->bf_state.bfs_paprd "after" calling ath_tx_setup_buffer?!
Quoted 4 lines
yes, i think its a problem, we cannot exactly say its the skb from mac80211 (or) its our PAPRD skb. though with the current code(as the PAPRD) is disabled the skb would be always from mac80211. let us see if we have some good solution.
got some idea, need to make sure this works perfectly, paprd frames are not aggregated, so we can look only at the non-aggregated path.
lets ignore aggregated path:
*ath_tx_form_aggr
*ath_tx_send_ampdu
non-aggregated path :
*ath_tx_start_dma
*ath_send_normal
-> 'ath_tx_setup_buffer' called from 'ath_tx_start_dma' can have a 'is_paprd' check based on 'txctl->paprd' which holds which chain we are sending PAPRD frame.
-> 'ath_send_normal' called from
-ath_tx_start_dma ( then 'bf' should be valid and we would not have called 'ath_tx_setup_buffer'
-ath_tx_flush_tid ( which is an aggregation path) ignore it.
Quoted 8 lines
Quoted 3 lines
attached rough patch for ath9k, need to be compiled and tested. i will also analyze if this can be done in a better way or any flaws in it
compilation fails with previous patch, forgot to declare the field in the declaration, attached patch passes compilation.

Quoted 1 line

would you like me to test this patch,

? Sure, I would like someone with a AR9300+ to test the patch. Currently, I'm stuck with a puny AR9287 [in a router]. So, I can't run tests all day :(.

Quoted 2 lines

i can just ran a bidirectional traffic test parallel y enabling PAPRD,with this proposed patch.

Actually, the changes are all within ath9k's tx error paths. So, I'm afraid just continuous traffic testing won't do, unless of course you can churn out all sorts of tx (dma) errors at the same time.

Regards,
Chr

Mohammed Shafi Shajakhan

2012-03-06 05:06 UTC #9 lore raw

RFC 1/2 ath9k: use ieee80211_free_txskb

Quoted 10 lines

Quoted 1 line
would you like me to test this patch,
? Sure, I would like someone with a AR9300+ to test the patch. Currently, I'm stuck with a puny AR9287 [in a router]. So, I can't run tests all day :(.
Quoted 2 lines
i can just ran a bidirectional traffic test parallel y enabling PAPRD,with this proposed patch.
Actually, the changes are all within ath9k's tx error paths. So, I'm afraid just continuous traffic testing won't do, unless of course you can churn out all sorts of tx (dma) errors at the same time.

yes, i can just a start stress test that can run few days in a noisy channel, with the AR9300 card.

Quoted 3 lines

Regards,
Chr

Signature · 2 lines

thanks,
shafi