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 v5 01/11] mt76: add hash lookup for skb on TXS status_list
To:     Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
References: <20210804134505.3208-1-greearb@candelatech.com>
 <ec49f403-54d3-28e1-3ea8-811df0756b29@nbd.name>
 <0a7e7206-91c0-35a9-8935-20bc6283367f@candelatech.com>
 <d479d24c-87b4-51c3-8f07-d71480913f8f@nbd.name>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <97657c37-8920-c8ba-a05c-28552ab71967@candelatech.com>
Date:   Fri, 13 Aug 2021 11:01:29 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <d479d24c-87b4-51c3-8f07-d71480913f8f@nbd.name>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-MW
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/13/21 10:46 AM, Felix Fietkau wrote:

>>>> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
>>>> index 6f302acb6e69..4c8504d3c904 100644
>>>> --- a/drivers/net/wireless/mediatek/mt76/tx.c
>>>> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
>>>> @@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
>>>>    			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
>>>>    		return MT_PACKET_ID_NO_SKB;
>>>>    
>>>> +	/* due to limited range of the pktid (7 bits), we can only
>>>> +	 * have a limited number of outstanding frames.  I think it is OK to
>>>> +	 * check the length outside of a lock since it doesn't matter too much
>>>> +	 * if we read wrong data here.
>>>> +	 * The TX-status callbacks don't always return a callback for an SKB,
>>>> +	 * so the status_list may contain some stale skbs.  Those will be cleaned
>>>> +	 * out periodically, see MT_TX_STATUS_SKB_TIMEOUT.
>>>> +	 */
>>>> +
>>>> +	qlen = skb_queue_len(&dev->status_list);
>>>> +	if (qlen > 120)
>>>> +		return MT_PACKET_ID_NO_SKB;
>>> Checking the length of the per-device status list doesn't make sense,
>>> since pktid allocation is per-wcid.
>>
>> Ok, so just remove this code, or should I set some other higher
>> limit to bound the list?
> You could just check for a duplicate skb_status_array entry.

Ok, that will happen anyway when searching for next wcid pkt-id.

The check above was a quick bail-out before locks were acquired.

I'll just remove that qlen check...

Thanks,
Ben

> 
> - Felix
> 


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