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

On Thu, 2017-05-04 at 13:13 +0000, Kalle Valo wrote:

quoted

quoted

quoted

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.

quoted

quoted

quoted

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