Re: [RFC 1/2] ath9k: use ieee80211_free_txskb

[Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>]

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

On Thursday 01 March 2012 05:12 PM, Mohammed Shafi Shajakhan wrote:
> On Thursday 01 March 2012 11:09 AM, Mohammed Shafi Shajakhan wrote:
>> Hi Christian,
>>
>> On Monday 27 February 2012 09:40 PM, Christian Lamparter wrote:
>>> 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<chunkeey@googlemail.com>
>>> ---
>>> 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?!
>>
>> 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
>

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


-- 
thanks,
shafi

[-- Attachment #2: use-mac80211-api-freeing-skb.patch --]
[-- Type: text/x-patch, Size: 3092 bytes --]

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 02e95c8..1a37a43 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1148,7 +1148,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)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 5dd27d2..7b9f1ba 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -64,7 +64,8 @@ static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
 static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
 					   struct ath_txq *txq,
 					   struct ath_atx_tid *tid,
-					   struct sk_buff *skb);
+					   struct sk_buff *skb,
+					   bool is_paprd);
 
 enum {
 	MCS_HT20,
@@ -811,7 +812,7 @@ static enum ATH_AGGR_STATUS ath_tx_form_aggr(struct ath_softc *sc,
 		fi = get_frame_info(skb);
 		bf = fi->bf;
 		if (!fi->bf)
-			bf = ath_tx_setup_buffer(sc, txq, tid, skb);
+			bf = ath_tx_setup_buffer(sc, txq, tid, skb, false);
 
 		if (!bf)
 			continue;
@@ -1728,7 +1729,7 @@ static void ath_tx_send_ampdu(struct ath_softc *sc, struct ath_atx_tid *tid,
 		return;
 	}
 
-	bf = ath_tx_setup_buffer(sc, txctl->txq, tid, skb);
+	bf = ath_tx_setup_buffer(sc, txctl->txq, tid, skb, false);
 	if (!bf)
 		return;
 
@@ -1754,8 +1755,9 @@ static void ath_tx_send_normal(struct ath_softc *sc, struct ath_txq *txq,
 	struct ath_buf *bf;
 
 	bf = fi->bf;
+
 	if (!bf)
-		bf = ath_tx_setup_buffer(sc, txq, tid, skb);
+		bf = ath_tx_setup_buffer(sc, txq, tid, skb, false);
 
 	if (!bf)
 		return;
@@ -1816,7 +1818,8 @@ u8 ath_txchainmask_reduction(struct ath_softc *sc, u8 chainmask, u32 rate)
 static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
 					   struct ath_txq *txq,
 					   struct ath_atx_tid *tid,
-					   struct sk_buff *skb)
+					   struct sk_buff *skb,
+					   bool is_paprd)
 {
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	struct ath_frame_info *fi = get_frame_info(skb);
@@ -1857,7 +1860,11 @@ static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
 	return bf;
 
 error:
-	dev_kfree_skb_any(skb);
+	if (is_paprd)
+		dev_kfree_skb_any(skb);
+	else
+		ieee80211_free_txskb(sc->hw, skb);
+
 	return NULL;
 }
 
@@ -1870,6 +1877,7 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct sk_buff *skb,
 	struct ath_atx_tid *tid = NULL;
 	struct ath_buf *bf;
 	u8 tidno;
+	bool is_paprd = false;
 
 	if ((sc->sc_flags & SC_OP_TXAGGR) && txctl->an &&
 		ieee80211_is_data_qos(hdr->frame_control)) {
@@ -1887,7 +1895,10 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct sk_buff *skb,
 		 */
 		ath_tx_send_ampdu(sc, tid, skb, txctl);
 	} else {
-		bf = ath_tx_setup_buffer(sc, txctl->txq, tid, skb);
+		if (txctl->paprd)
+			is_paprd = true;
+
+		bf = ath_tx_setup_buffer(sc, txctl->txq, tid, skb, is_paprd);
 		if (!bf)
 			return;