Date:   Tue, 19 Mar 2019 12:07:09 +0100
From:   Stanislaw Gruszka <sgruszka@redhat.com>
To:     Lorenzo Bianconi <lorenzo@kernel.org>
Cc:     nbd@nbd.name, linux-wireless@vger.kernel.org,
        lorenzo.bianconi@redhat.com
Subject: Re: [RFC] mt76: usb: reduce locking in mt76u_tx_tasklet
Message-ID: <20190319110708.GA5360@redhat.com>
References: <cover.1552907276.git.lorenzo@kernel.org>
 <1ee5ce7818f9d45c9713ce99e810cb84f50dcf03.1552907276.git.lorenzo@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1ee5ce7818f9d45c9713ce99e810cb84f50dcf03.1552907276.git.lorenzo@kernel.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 18, 2019 at 12:09:32PM +0100, Lorenzo Bianconi wrote:
> Similar to pci counterpart, reduce locking in mt76u_tx_tasklet since
> q->head is managed just in mt76u_tx_tasklet and q->queued is updated
> holding q->lock
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/wireless/mediatek/mt76/usb.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index ac03acdae279..8cd70c32d77a 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -634,29 +634,33 @@ static void mt76u_tx_tasklet(unsigned long data)
>  	int i;
>  
>  	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
> +		u32 n_queued = 0, n_sw_queued = 0;
> +
>  		sq = &dev->q_tx[i];
>  		q = sq->q;
>  
> -		spin_lock_bh(&q->lock);
> -		while (true) {
> +		while (q->queued > n_queued) {
>  			buf = &q->entry[q->head].ubuf;
> -			if (!buf->done || !q->queued)
> +			if (!buf->done)
>  				break;

I'm still thinking if this is safe or not. Is somewhat tricky to
read variable outside the lock because in such case there is no time
guarantee when variable written on one CPU gets updated value on
different CPU. And for USB is not only q->queued but also buf->done.
 
Stanislaw