On 21 January 2014 16:06, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote: >> The patch improves channel switch related locking >> (STA, IBSS, AP, mesh). >> >> Now read access to sdata->vif.csa_active is >> protected by wdev.mtx and local->mtx so holding >> either is enough for read access but both are >> required for write access. Keep in mind sdata lock >> must be taken before local->mtx. Taking them in >> reverse order creates a deadlock situation. >> >> The only exception is ieee80211_beacon_get_tim() >> but it's safe to leave it as is and it doesn't >> influence mac80211 state in any way. >> >> The patch adds a few lockdep assertions along for >> easier code/locking maintenance. > --- >> This also prepares for multi-interface CSA. > > There's only one or two lines for that preparation as far as I can tell, > but I think you should separate it (or maybe make that part of another > patch)... or did I miss something? True. I'll remove the sentence. > I mean this: > >> + sdata->csa_chandef = params.chandef; > > doesn't seem to belong here. I mistakenly left that while rebasing. I'll fix that. I think that's the only thing that shouldn't be in this patch? > It would also be good to explain why you're doing this? You mean the whole locking patch? Basically to be able safely assess CSA state. Taking a bunch of wdev->mtx seems like a bad idea. Due to how the locks are ordered and organized I've came up with using local->mtx as an extra CSA-related variable protection. Using local->mtx makes it possible to iterate over interfaces and check for/update CSA state in an atomic way. Is this a satisfactory explanation? Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
