diff mbox series

mt76: Ensure sale skb status list is processed.

Message ID 20220121195548.17476-1-greearb@candelatech.com
State New
Headers show
Series mt76: Ensure sale skb status list is processed. | expand

Commit Message

Ben Greear Jan. 21, 2022, 7:55 p.m. UTC
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(-)

Comments

Ben Greear Jan. 22, 2022, 1:07 a.m. UTC | #1
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))
>
Lorenzo Bianconi Jan. 22, 2022, 11:22 a.m. UTC | #2
> 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
>
diff mbox series

Patch

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