linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stanislaw Gruszka <sgruszka@redhat.com>
To: Gertjan van Wingerde <gwingerde@gmail.com>
Cc: "John W. Linville" <linville@tuxdriver.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"users@rt2x00.serialmonkey.com" <users@rt2x00.serialmonkey.com>
Subject: Re: [rt2x00-users] [PATCH 3.3] rt2x00: fix random stalls
Date: Tue, 6 Mar 2012 07:53:30 +0100	[thread overview]
Message-ID: <20120306065329.GA2373@redhat.com> (raw)
In-Reply-To: <8287E603-C5DE-4CF9-8647-DE63B9D3E93E@gmail.com>

Hi Gertjan

On Mon, Mar 05, 2012 at 08:54:37PM +0100, Gertjan van Wingerde wrote:
> There are more places in the rt2x00 code that call upon rt2x00queue_pause_queue and rt2x00queue_unpause_queue. Shouldn't these places be protected with tx_lock as well?

Hmm, good question. Perhaps there are possible races between other usage
of pause/unpause, but they are not obvious for me. 

Seems there are more bugs there, i.e on rt2x00queue_stop_queue, we first
clear QUEUE_STARTED then call pause_queue, which will just return with
that bit cleared.

On rt2x00queue_flush_queue(), we first call pause queue, then
->kick_queue, which will cause to call txdone and unpause queue. 

So, it's hard to tell for me right now if other paces needs serialization,
apparently related area needs some more detailed review, 

> Or better, shouldn't the locking be moved inside the pause / unpause functions?

Thats a bit more complication, because we need lock for TX queues only
and it has to be taken before test_and_{set,clear}_bit(QUEUE_PAUSED, &queue->flags),
otherwise we still can race.

I believe this patch is simplest possible approach to solve the problem,
at least for now.

Stanislaw

  reply	other threads:[~2012-03-06  6:53 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-05 16:48 [PATCH 3.3] rt2x00: fix random stalls Stanislaw Gruszka
2012-03-05 19:27 ` [rt2x00-users] " Ivo Van Doorn
2012-03-05 19:54 ` Gertjan van Wingerde
2012-03-06  6:53   ` Stanislaw Gruszka [this message]
2012-03-06  7:45 ` Helmut Schaa
2012-03-06 11:53   ` Stanislaw Gruszka
2012-03-06 12:08     ` Gertjan van Wingerde
2012-03-07 18:25       ` Stanislaw Gruszka
     [not found] ` <4F564CDC.5040808@gmail.com>
2012-03-06 21:37   ` Martin Hundebøll
2012-03-07 18:46   ` Stanislaw Gruszka
2012-03-11  9:53     ` Martin Hundebøll

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120306065329.GA2373@redhat.com \
    --to=sgruszka@redhat.com \
    --cc=gwingerde@gmail.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linville@tuxdriver.com \
    --cc=users@rt2x00.serialmonkey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).