linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Brian Norris <briannorris@chromium.org>
Cc: Kalle Valo <kvalo@codeaurora.org>,
	Ganapathi Bhat <gbhat@marvell.com>,
	Nishant Sarmukadam <nishants@marvell.com>,
	linux-kernel@vger.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Amitkumar Karwar <amitkarwar@gmail.com>,
	linux-wireless@vger.kernel.org
Subject: Re: [PATCH 05/14] mwifiex: re-register wiphy across reset
Date: Wed, 28 Jun 2017 09:28:49 +0200	[thread overview]
Message-ID: <1498634929.12531.3.camel@sipsolutions.net> (raw)
In-Reply-To: <20170627204858.GB93674@google.com> (sfid-20170627_224903_828027_554AFBFB)

On Tue, 2017-06-27 at 13:48 -0700, Brian Norris wrote:

> > There isn't really a good way to do this. You can, of course, call
> > wiphy_unregister(), but if you could do that you'd already have the
> > problem solved, I think?
> 
> That's probably along the right track. There are still some things
> we'd need to do properly before that though, and this is where all
> the problems are so far. (Also, this is what Kalle was already
> objecting to; he didn't think we should be unregistering/recreating
> the wiphy, but I think he ended up softening on that a bit.)
> 
> For one, I still expect I should be removing the wireless dev's
> before unregistering the wihpy, no? Otherwise, there will be existing
> wdevs backed by an unregistered wiphy?

Yeah, that's true - though once you get rid of those they can't be
accessed any more.

> And that gets to the heart of another bug: deleting interfaces (e.g.,
> "iw dev foo del") races with a lot of stuff -- like see
> 
> mwifiex_process_sta_event() ->
>   EVENT_EXT_SCAN_REPORT ->
>     netif_running(priv->netdev)
> 
> Because mwifiex_del_virtual_intf() doesn't stop any outstanding
> commands, we can be both deleting the netdev and processing scans for
> it.

Huh, well, I guess you need some kind of locking here anyway, since the
user can always do things like deleting the interface while a scan is
running?

> > > Also, IIUC, we need to wait for all control paths to complete (or
> > > cancel) before we can free up the associated resources; so just
> > > marking "unavailable" isn't enough.
> > 
> > Yeah, I suppose so. Though if you just do all the freeing after
> > wiphy_unregister() it'll do that for you?
> 
> Yes, I think so. Then part of the problem is probably that some of
> the current "cancel command" logic is tied up with the "free command
> structures" logic. So we're freeing some stuff too early.
> 
> Anyway, those sorts of bugs aside, IIUC the full sequence for
> teardown should probably be something like:
> 
> 1. Stop TX queues
> 2. Cancel outstanding commands (let them fail or finish, etc.) -- but
>    DON'T free their backing resources yet
> 3. Remove wdevs
> 4. wiphy_unregister()
> 5. Free up resources
> 
> Current problems are at least:
> 
> * we don't do step 4 in the right place (if at all; see this patch)
> * step 2 mixes in "free"ing resources too early

So I'm not sure what you mean by splitting in 2/5 - this seems
reasonable, but I don't understand why something like a scan request
wouldn't be freed while you cancel it in 2? In fact, you really have to
free it before you remove the corresponding wdev, or cfg80211 will
complain?

johannes

  reply	other threads:[~2017-06-28  7:28 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-25  0:11 [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks Brian Norris
2017-05-25  0:11 ` [PATCH 02/14] mwifiex: reunite copy-and-pasted remove/reset code Brian Norris
2017-05-25  0:11 ` [PATCH 03/14] mwifiex: reset interrupt status across device reset Brian Norris
2017-05-25  0:11 ` [PATCH 04/14] mwifiex: pcie: don't allow cmd buffer reuse after reset Brian Norris
2017-05-25  0:11 ` [PATCH 05/14] mwifiex: re-register wiphy across reset Brian Norris
2017-06-01  9:15   ` Kalle Valo
2017-06-01 17:39     ` Brian Norris
2017-06-05 15:54       ` Kalle Valo
2017-06-09  9:03         ` Johannes Berg
2017-06-21 18:27           ` Brian Norris
2017-06-22 13:02             ` Johannes Berg
2017-06-27 20:48               ` Brian Norris
2017-06-28  7:28                 ` Johannes Berg [this message]
2017-06-29 18:45                   ` Brian Norris
2017-07-04 14:10                 ` Kalle Valo
2017-06-21 17:48         ` Brian Norris
2017-06-22 12:59           ` Johannes Berg
2017-06-27 19:50             ` Brian Norris
2017-06-28  7:21               ` Johannes Berg
2017-05-25  0:11 ` [PATCH 06/14] mwifiex: don't short-circuit netdev notifiers on interface deletion Brian Norris
2017-05-25  0:11 ` [PATCH 07/14] mwifiex: fixup init_channel_scan_gap error case Brian Norris
2017-05-25  0:11 ` [PATCH 08/14] mwifiex: ensure "disable auto DS" struct is initialized Brian Norris
2017-05-25  0:11 ` [PATCH 09/14] mwifiex: pcie: remove redundant synchronize_irq() Brian Norris
2017-05-25  0:11 ` [PATCH 10/14] mwifiex: pcie: stop masking interrupts in FW downloader Brian Norris
2017-05-25  0:11 ` [PATCH 11/14] mwifiex: utilize netif_tx_{wake,stop}_all_queues() Brian Norris
2017-05-25  0:11 ` [PATCH 12/14] mwifiex: don't open-code ARRAY_SIZE() Brian Norris
2017-05-25  0:11 ` [PATCH 13/14] mwifiex: drop 'add_tail' param from mwifiex_insert_cmd_to_pending_q() Brian Norris
2017-05-25  0:11 ` [PATCH 14/14] mwifiex: pcie: fix whitespace Brian Norris
2017-05-31 17:10 ` [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks Brian Norris
2017-06-05 11:49   ` Xinming Hu

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=1498634929.12531.3.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=amitkarwar@gmail.com \
    --cc=briannorris@chromium.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=gbhat@marvell.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=nishants@marvell.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).