On 7/30/20 6:41 AM, Johannes Berg wrote:
On Thu, 2020-07-30 at 06:27 -0700, Ben Greear wrote:
1. track per vif whether it was re-added, and skip before it is
If that works, I can certainly get behind it for semantic reasons (the
vif isn't yet active again), although even there I'm not sure how
iwlwifi would behave - but that's something I'd look into and perhaps
even consider a bug there since it shouldn't know about that interface
yet.
Wouldn't that complicate the sdata-in-driver check even more? So it would
be something like is-in-driver-but-not-yet-reconfigured-in-driver?
We should probably just clear the "is-in-driver" flag for the most part,
and just remember "was-in-driver" so we know which ones to reconfigure,
or something like that?
This sort of state is quite nasty in my experience. Almost better if
we had less state. Driver should already know if it has an object or not,
so redundant adds, or requests from mac80211 for objects the driver does not
have can be handled properly by the driver?
I don't think that'll work. Drivers just act on "add" and "remove",
they're not checking against double-add. And IMHO it makes more sense to
have mac80211 do good sequencing than the throw our hands in the air and
let the drivers have to be idempotent just because we can't figure out
the right sequencing?
They certainly could check. At the least, removing something that is already
gone should be an easy check. That would at least let mac80211 be more confident
about cleanup. The current in-kernel code that cleared SDATA_IN_DRIVER in
the non-recoverable firmware crash bit just assumed that drivers would clean
things up, but I'm not even sure they can entirely clean things up since often
the objects in their local memory are held by mac80211 objects (ie, the priv
opaque objects).
2. If for some reason that doesn't work, add an iteration flag that
controls this, rather than a per-device config?
I wrote this patch probably 5 or so years ago, and since I have fixed most of
the ath10k firmware crashes that would tend to trigger this bug. I think I have no
good way to test any complicated change to this patch.
I am quite certain that ath9k and ath10k are at least not harmed by this patch, and
certainly old ath10k benefited from this. So, I'm comfortable adding a driver
level flag enabled for those two drivers. Changing all the calling locations to
(maybe) add a flag or adding and tracking vif state is too risky to be worth this change I
think.
Uh, why? I mean, at least mechanically replacing all the callers in that
driver wouldn't really be any different than adding a driver flag, but
is so much more flexible and can be used elsewhere. I don't think I buy
this argument much really.
Yes, I understand that there's always resistance to changing something
that works, but ...
Really I think the right thing to do here would be 1., and let mac80211
sort out the sequencing.
Consider
add interface wlan0
add interface wlan1
iterate active interfaces -> wlan0 wlan1
add interface wlan2
iterate active interfaces -> wlan0 wlan1 wlan2
If you apply this scenario to a restart, which ought to be functionally
equivalent to the normal startup, just compressed in time, you're
basically saying that today you get
add interface wlan0
add interface wlan1
iterate active interfaces -> wlan0 wlan1 wlan2 << problem here
add interface wlan2
iterate active interfaces -> wlan0 wlan1 wlan2
which yeah, totally seems wrong.
But fixing that to be
add interface wlan0
add interface wlan1
iterate active interfaces ->
<nothing>
add interface wlan2
iterate active interfaces -> <nothing>
(or
maybe -> wlan0 wlan1 wlan2 if the reconfig already completed)
seems equally wrong?
So, looks like there is a flags option passed to the iterate logic, and it is indeed called
directly from drivers. So, I could just add a new flag value, and | it in when calling from ath10k.
I'm not sure it would really solve the second case, but at least in practice,
that one doesn't seem to be a problem with ath10k, and the first case *was*
a problem.
If that sounds OK to you, I'll work on the patch as described.
Thanks,
Ben
johannes
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
