Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676418

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Brian Norris <briannorris@chromium.org> - 2017-06-21 20:30 +0200
  Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Johannes Berg <johannes@sipsolutions.net> - 2017-06-22 15:10 +0200
    Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Brian Norris <briannorris@chromium.org> - 2017-06-27 22:50 +0200
      Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Johannes Berg <johannes@sipsolutions.net> - 2017-06-28 09:30 +0200
        Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Brian Norris <briannorris@chromium.org> - 2017-06-29 20:50 +0200

csiph-web