Re: [PATCH 2/4] wcn36xx: Add TX ack support

[Loic Poulain <loic.poulain@linaro.org>]

On Mon, 20 Jul 2020 at 19:19, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Loic Poulain <loic.poulain@linaro.org> writes:
>
> > The controller is capable of reporting TX indication which can be used
> > to report TX ack. It was only partially implemented.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>
> [...]
>
> > +static void wcn36xx_dxe_tx_timer(struct timer_list *t)
> > +{
> > +     struct wcn36xx *wcn = from_timer(wcn, t, tx_ack_timer);
> > +     struct ieee80211_tx_info *info;
> > +     unsigned long flags;
> > +     struct sk_buff *skb;
> > +
> > +     /* TX Timeout */
> > +     wcn36xx_dbg(WCN36XX_DBG_DXE, "TX timeout\n");
> > +
> > +     spin_lock_irqsave(&wcn->dxe_lock, flags);
> > +     skb = wcn->tx_ack_skb;
> > +     wcn->tx_ack_skb = NULL;
> > +     spin_unlock_irqrestore(&wcn->dxe_lock, flags);
> > +
> > +     if (!skb)
> > +             return;
> > +
> > +     info = IEEE80211_SKB_CB(skb);
> > +     info->flags &= ~IEEE80211_TX_STAT_ACK;
> > +     info->flags &= ~IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> > +
> > +     ieee80211_tx_status_irqsafe(wcn->hw, skb);
> > +     ieee80211_wake_queues(wcn->hw);
> > +}
>
> What's this timer thing? The commit log mentions nothing about that. To
> me this looks like a some kind of TX timeout detection and has nothing
> to do ACK handling, but of course I might have misunderstood.
>
> Should it be in a separate patch to follow one logical change per patch
> rule?

This is part of ack management, I could have named it dex_ack_timer though...
When we send a packet requesting explicit ack notification via mac80211
status callback, we ask the firmware to return an ack event and stop tx queue
temporarily until the ack event is received. But if for any reasons the 802.11
packet is not acked, firmware never sends this event, causing stale of the
TX path.

So the timer is here to handle the no-ack case, in order to
1. run the mac80211 status callback (packet not acked)
2. unblock TX queue

> > --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> > @@ -45,8 +45,8 @@ static struct wcn36xx_cfg_val wcn36xx_cfg_vals[] = {
> >       WCN36XX_CFG_VAL(MAX_MEDIUM_TIME, 6000),
> >       WCN36XX_CFG_VAL(MAX_MPDUS_IN_AMPDU, 64),
> >       WCN36XX_CFG_VAL(RTS_THRESHOLD, 2347),
> > -     WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 6),
> > -     WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 6),
> > +     WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 15),
> > +     WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 15),
>
> Also there's no mention of these changes in the commit log. Should these
> in a third patch as they are more like a separate change?

Correct, this change increases the number of TX retries, to improve robustness,
and have more chances to have a packet acked. 15 is the default value defined
by the downstream driver. I observed much less ack timeout in a noisy
environment with that change.

So I can rework the commit for with ack timer info, and move the
config change in a separate change.

Regards,
Loic