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

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

[greearb]

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.

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);
+		__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

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

[Ben Greear]

On 1/21/22 11:55 AM, greearb@candelatech.com wrote:
> 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.

Please ignore this, I did not understand how the mac_work logic could
call the tx_status_skb_get such that pktid was purposefully invalid
and thus cause the cleanup logic to happen.

Perhaps my original issue this attempted to fix was related to
some other problem.

Thanks,
Bne

> 
> 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);
> +		__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))
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Lorenzo Bianconi]

[-- 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 --]