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? > > 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? johannes
