Re: [PATCH] mt76: Ensure sale skb status list is processed.

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3340 bytes --]

> From: Ben Greear <greearb@candelatech.com>
> 
> The old code might not ever run the stale skb status processing
> list, so change it to ensure the stale entries are cleaned up
> regularly.

I guess this work is already performed in mt76_tx_status_check() executed by
mac work (e.g. mt7921_mac_work()) where pid is set to 0 and the first lookup
will always fail. Have you identified an issue in the current codebase?

> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
>  drivers/net/wireless/mediatek/mt76/tx.c   | 24 +++++++++++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 37d82d806c09..bfb68788251f 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -271,6 +271,7 @@ struct mt76_wcid {
>  
>  	struct list_head list;
>  	struct idr pktid;
> +	unsigned long last_idr_check_at; /* in jiffies */
>  };
>  
>  struct mt76_txq {
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> index 938353ac272f..b6f0d74fd563 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -157,24 +157,35 @@ mt76_tx_status_skb_get(struct mt76_dev *dev, struct mt76_wcid *wcid, int pktid,
>  		       struct sk_buff_head *list)
>  {
>  	struct sk_buff *skb;
> +	struct sk_buff *skb2;
>  	int id;
> +	/* Check twice as often as the timeout value so that we mitigate
> +	 * worse-case timeout detection (where we do the check right before
> +	 * the per skb timer would have expired and so have to wait another interval
> +	 * to detect the skb status timeout.)
> +	 */
> +	static const int check_interval = MT_TX_STATUS_SKB_TIMEOUT / 2;
>  
>  	lockdep_assert_held(&dev->status_lock);
>  
>  	skb = idr_remove(&wcid->pktid, pktid);
> -	if (skb)
> +
> +	/* If we have not checked for stale entries recently,
> +	 * then do that check now.
> +	 */
> +	if (time_is_after_jiffies(wcid->last_idr_check_at + check_interval))
>  		goto out;
>  
>  	/* look for stale entries in the wcid idr queue */
> -	idr_for_each_entry(&wcid->pktid, skb, id) {
> -		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
> +	idr_for_each_entry(&wcid->pktid, skb2, id) {
> +		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb2);
>  
>  		if (pktid >= 0) {
>  			if (!(cb->flags & MT_TX_CB_DMA_DONE))
>  				continue;
>  
>  			if (time_is_after_jiffies(cb->jiffies +
> -						   MT_TX_STATUS_SKB_TIMEOUT))
> +						  MT_TX_STATUS_SKB_TIMEOUT))
>  				continue;
>  		}
>  
> @@ -182,9 +193,10 @@ mt76_tx_status_skb_get(struct mt76_dev *dev, struct mt76_wcid *wcid, int pktid,
>  		 * and stop waiting for TXS callback.
>  		 */
>  		idr_remove(&wcid->pktid, cb->pktid);
> -		__mt76_tx_status_skb_done(dev, skb, MT_TX_CB_TXS_FAILED |
> -						    MT_TX_CB_TXS_DONE, list);

I guess it is more readable as it was before.

Regards,
Lorenzo

> +		__mt76_tx_status_skb_done(dev, skb2, MT_TX_CB_TXS_FAILED |
> +					  MT_TX_CB_TXS_DONE, list);
>  	}
> +	wcid->last_idr_check_at = jiffies;
>  
>  out:
>  	if (idr_is_empty(&wcid->pktid))
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]