[1/2] wcn36xx: Pass used skb to ieee80211_tx_status()

Message ID 20170426220444.10539-1-bjorn.andersson@linaro.org
State New
Headers show
Series
  • [1/2] wcn36xx: Pass used skb to ieee80211_tx_status()
Related show

Commit Message

Bjorn Andersson April 26, 2017, 10:04 p.m.
As the tx skbs are collected they should be passed to
ieee80211_tx_status() rather than ieee80211_free_txskb(), as the prior
will take care of monitoring and LED triggers while the latter will
consider the skb dropped.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Andersson April 28, 2017, 11:42 p.m. | #1
On Thu 27 Apr 01:22 PDT 2017, Johannes Berg wrote:

> 

> > @@ -371,7 +371,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn,

> > struct wcn36xx_dxe_ch *ch)

> >  			info = IEEE80211_SKB_CB(ctl->skb);

> >  			if (!(info->flags &

> > IEEE80211_TX_CTL_REQ_TX_STATUS)) {

> >  				/* Keep frame until TX status comes

> > */

> > -				ieee80211_free_txskb(wcn->hw, ctl-

> > >skb);

> > +				ieee80211_tx_status(wcn->hw, ctl-

> > >skb);

> > 

> 

> I don't think this is a good idea.


Thanks for letting me know :)

> This code intentionally checked if TX status was requested, and if not

> then it doesn't go to the effort of building it.

> 


What I'm finding puzzling is the fact that the only caller of
ieee80211_led_tx() is ieee80211_tx_status() and it seems like drivers,
such as ath10k, call this for each packet handled - but I'm likely
missing something.

> As it is with your patch, it'll go and report the TX status without any

> TX status information - which is handled in wcn36xx_dxe_tx_ack_ind()

> for those frames needing it.

> 


Right, it doesn't sound desired. However, during normal operation I'm
not seeing IEEE80211_TX_CTL_REQ_TX_STATUS being set and as such
ieee80211_led_tx() is never called.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kalle Valo May 4, 2017, 1:13 p.m. | #2
Bjorn Andersson <bjorn.andersson@linaro.org> writes:

> On Thu 27 Apr 01:22 PDT 2017, Johannes Berg wrote:

>

>> 

>> > @@ -371,7 +371,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn,

>> > struct wcn36xx_dxe_ch *ch)

>> >  			info = IEEE80211_SKB_CB(ctl->skb);

>> >  			if (!(info->flags &

>> > IEEE80211_TX_CTL_REQ_TX_STATUS)) {

>> >  				/* Keep frame until TX status comes

>> > */

>> > -				ieee80211_free_txskb(wcn->hw, ctl-

>> > >skb);

>> > +				ieee80211_tx_status(wcn->hw, ctl-

>> > >skb);

>> > 

>> 

>> I don't think this is a good idea.

>

> Thanks for letting me know :)

>

>> This code intentionally checked if TX status was requested, and if not

>> then it doesn't go to the effort of building it.

>> 

>

> What I'm finding puzzling is the fact that the only caller of

> ieee80211_led_tx() is ieee80211_tx_status() and it seems like drivers,

> such as ath10k, call this for each packet handled - but I'm likely

> missing something.

>

>> As it is with your patch, it'll go and report the TX status without any

>> TX status information - which is handled in wcn36xx_dxe_tx_ack_ind()

>> for those frames needing it.

>> 

>

> Right, it doesn't sound desired. However, during normal operation I'm

> not seeing IEEE80211_TX_CTL_REQ_TX_STATUS being set and as such

> ieee80211_led_tx() is never called.


So what's the conclusion? How do we get leds working?

-- 
Kalle Valo

Patch hide | download patch | download mbox

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 87dfdaf9044c..938b7bd733cf 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -371,7 +371,7 @@  static void reap_tx_dxes(struct wcn36xx *wcn, struct wcn36xx_dxe_ch *ch)
 			info = IEEE80211_SKB_CB(ctl->skb);
 			if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)) {
 				/* Keep frame until TX status comes */
-				ieee80211_free_txskb(wcn->hw, ctl->skb);
+				ieee80211_tx_status(wcn->hw, ctl->skb);
 			}
 			spin_lock(&ctl->skb_lock);
 			if (wcn->queues_stopped) {