Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] mt76: Ensure sale skb status list is processed.
To:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
References: <20220121195548.17476-1-greearb@candelatech.com>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <256db504-e4f5-21e0-b621-c086f73f88c5@candelatech.com>
Date:   Fri, 21 Jan 2022 17:07:00 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <20220121195548.17476-1-greearb@candelatech.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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