Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631754 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status() Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-27 00:10 +0200
    [PATCH 2/2] arm64: dts: apq8016-sbc: Correct WLAN LED default-trigger Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-27 00:10 +0200
    Re: [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status() Johannes Berg <johannes@sipsolutions.net> - 2017-04-27 10:30 +0200
      Re: [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status() Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-29 01:50 +0200
        Re: [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status() Kalle Valo <kvalo@qca.qualcomm.com> - 2017-05-04 15:20 +0200

#1631754 — [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status()

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(-)

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) {
-- 
2.12.0

[toc] | [next] | [standalone]

#1631756 — [PATCH 2/2] arm64: dts: apq8016-sbc: Correct WLAN LED default-trigger

The TX status trigger of the wlan interface is named phy0tx, so this
updates the default-trigger for the WLAN LED to use that instead.

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

Note that without patch 1/2 this trigger does not fire - but there's also no
harm in picking the two patches through separate trees.

 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 5d83b02b7c4a..21a8f5ce8955 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -178,7 +178,7 @@
 			led@5 {
 				label = "apq8016-sbc:yellow:wlan";
 				gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
-				linux,default-trigger = "wlan";
+				linux,default-trigger = "phy0tx";
 				default-state = "off";
 			};
 
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1631948

> @@ -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. This code intentionally checked if
TX status was requested, and if not then it doesn't go to the effort of
building it.

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.

johannes

[toc] | [prev] | [next] | [standalone]

#1633205

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

[toc] | [prev] | [next] | [standalone]

#1635726

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web