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

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

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

[Johannes Berg]

> @@ -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

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

[Bjorn Andersson]

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

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

[Kalle Valo]

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

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

[Johannes Berg]

On Thu, 2017-05-04 at 13:13 +0000, Kalle Valo wrote:
> 
> > > 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.

Yes, many drivers do call it for each packet, and as such, this
deficiency was never noted.

> > > 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?

Well, frankly, I never thought the TX LED was a super good idea - but
it had been supported by the original code IIRC, so never removed. Some
people like frantic blinking I guess ;-)

But I think the problem also applies to the throughput trigger thing,
so perhaps we need to stick some LED feedback calls into other places,
like _noskb() or provide an extra way to do it?

johannes

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

[Bjorn Andersson]

On Wed 17 May 06:14 PDT 2017, Johannes Berg wrote:

> On Thu, 2017-05-04 at 13:13 +0000, Kalle Valo wrote:
> > 
> > > > 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.
> 
> Yes, many drivers do call it for each packet, and as such, this
> deficiency was never noted.
> 
> > > > 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?
> 
> Well, frankly, I never thought the TX LED was a super good idea - but
> it had been supported by the original code IIRC, so never removed. Some
> people like frantic blinking I guess ;-)
> 

It seems very important to a lot of people...


But if ieee80211_free_txskb() is the counterpart of
ieee80211_tx_status() then we should be able to push the
ieee80211_led_tx() call down into ieee80211_report_used_skb() and handle
both cases.

The ieee80211_free_txskb() seems to be used in various cases where we
discard skbs, but perhaps this is not an issue in reality.

> But I think the problem also applies to the throughput trigger thing,
> so perhaps we need to stick some LED feedback calls into other places,
> like _noskb() or provide an extra way to do it?
> 

Looking around it seems that we either have a call to free_txskb() or
one of the tx_status(); where the _noskb() would need some special
handling. Are there others or would it be reasonable to add a call in
this one "special" case?

Regards,
Bjorn

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

[Johannes Berg]

On Wed, 2017-05-17 at 22:05 -0700, Bjorn Andersson wrote:
> 
> It seems very important to a lot of people...

I get blinking, I guess, but I don't get toggling for every packet :)
The throughput thing we did in iwlwifi seems like a so much better
idea. Not that it really matters for this discussion though.

> But if ieee80211_free_txskb() is the counterpart of
> ieee80211_tx_status() then we should be able to push the
> ieee80211_led_tx() call down into ieee80211_report_used_skb() and
> handle both cases.

Yeah, I guess that works.

> The ieee80211_free_txskb() seems to be used in various cases where we
> discard skbs, but perhaps this is not an issue in reality.

Those should be code paths that are really rare, when we fail
allocations in some places, etc. So it shouldn't really lead to any
problems.

> Looking around it seems that we either have a call to free_txskb() or
> one of the tx_status(); 

Yes, you're right - we always need one of those for each SKB that
passed through mac80211, everything else is already a bug.

> where the _noskb() would need some special
> handling. Are there others or would it be reasonable to add a call in
> this one "special" case?

Now that I think more about it, the _noskb() doesn't actually make
sense - it's for a separate status report, pretty much only for rate
control feedback, but the SKB should be freed separately with
free_txskb().

johannes