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

Groups > linux.kernel > #1671855 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Brian Norris <briannorris@chromium.org> - 2017-06-21 19:50 +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:00 +0200
        Re: [PATCH 05/14] mwifiex: re-register wiphy across reset Johannes Berg <johannes@sipsolutions.net> - 2017-06-28 09:30 +0200

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

Hi Kalle (and Johannes; I'll reply to Johannes response separately too),

On Mon, Jun 05, 2017 at 06:54:18PM +0300, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> writes:
> > That's not to say that there aren't such bugs out there. I'd still be
> > willing to bet there are. And IMO, it seems wise to just do the same
> > teardown/setup as one would do for (e.g.) 'rmmod', to prevent exposing
> > *too* many new permutations of "wiphy is available but rest of the
> > driver is torn down".
> 
> This feels like a sledge hammer approach causing all sort of problems

Yes, it is a sledge hammer. But I'm working with what we have here. With
this approach, it's also easier to tell that things aren't out-of-sync,
since I'm never quite sure how much state was held in the firmware (and
now won't match what user space thinks). A full removal / re-init makes
this clear -- user space should expect *everything* to be reset.

I'm open to learning better approaches if possible, but this also might
be difficult if I don't get any support from Marvell on this. (They seem
quite happy to let sleeping dogs lie.)

> for user space and I really like the mac80211 approach more. For
> example, if an ath10k firmware crash happens user only sees a few second
> pause in data traffic and a warning in kernel log, otherwise everything
> happens behind the scenes. Of course there are very likely races
> somewhere but at least I haven't seen that many reports related to
> firmware restart functionality.

Yes, that all sounds nice. But for my sake, can you describe better
what's actually going on there (e.g., can you point me at which code
does this)? I'm really not familiar with mac80211 (though I was aware of
the above general behavior). But to my knowledge, mac80211 drivers keep
a lot more state managed in the kernel, so it's a little easier and
more natural to get the driver/FW back to "the same state" than it is
with a full-MAC driver.

> > But if none of this is convincing to you, I can take a stab at a
> > different solution.
> 
> I don't have any problem applying this patch but more about being
> curious why doing it like this. And hopefully finding a less intrusive
> solution in the future.

OK, sure. I'll see what I can do, but I don't see an easy path at the
moment toward fixing (i.e., completely rewriting) this long-standing
driver behavior.

[trim]

Thanks,
Brian

[toc] | [next] | [standalone]

#1672609

On Wed, 2017-06-21 at 10:48 -0700, Brian Norris wrote:
> 
> Yes, that all sounds nice. But for my sake, can you describe better
> what's actually going on there (e.g., can you point me at which code
> does this)? 

It's much easier with mac80211, it has all the state. Basically the
reconfig is in ieee80211_reconfig() :)

> I'm really not familiar with mac80211 (though I was aware of
> the above general behavior). But to my knowledge, mac80211 drivers
> keep a lot more state managed in the kernel, so it's a little easier
> and more natural to get the driver/FW back to "the same state" than
> it is with a full-MAC driver.

Indeed.

johannes

[toc] | [prev] | [next] | [standalone]

#1676127

(A little slow on follow-up here)

On Thu, Jun 22, 2017 at 02:59:49PM +0200, Johannes Berg wrote:
> On Wed, 2017-06-21 at 10:48 -0700, Brian Norris wrote:
> > 
> > Yes, that all sounds nice. But for my sake, can you describe better
> > what's actually going on there (e.g., can you point me at which code
> > does this)? 
> 
> It's much easier with mac80211, it has all the state. Basically the
> reconfig is in ieee80211_reconfig() :)

Wow, that's not exactly simple code; I expect it could be pretty
difficult to get that right today on mwifiex. The current approach
actually should be *easier* (for the kernel side) to avoid bugs, as it
should be basically the same thing as 'rmmod'. Nonetheless, there are
plenty of bugs.

Thanks for the pointer though.

> > I'm really not familiar with mac80211 (though I was aware of
> > the above general behavior). But to my knowledge, mac80211 drivers
> > keep a lot more state managed in the kernel, so it's a little easier
> > and more natural to get the driver/FW back to "the same state" than
> > it is with a full-MAC driver.
> 
> Indeed.

Brian

[toc] | [prev] | [next] | [standalone]

#1676419

Hi Brian,

> Wow, that's not exactly simple code; I expect it could be pretty
> difficult to get that right today on mwifiex. 

Yeah, I have no doubt. You'd probably have to track a lot of state that
you just pass down to the firmware too, and possibly can't even track
some state that the firmware derives itself (like for example PNs for
keys)

> The current approach
> actually should be *easier* (for the kernel side) to avoid bugs, as
> it should be basically the same thing as 'rmmod'. Nonetheless, there
> are plenty of bugs.

:-)

johannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web