On 22 January 2014 09:52, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > On Wed, 2014-01-22 at 07:51 +0100, Michal Kazior wrote: > >> > 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's the only thing I found, but ... :) > >> > 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? > > Almost - where exactly do you need that? I'm just trying to weigh this > patch vs. the other that I didn't like to see if this is needed > regardless of the particular approach of how we switch channels later. Hmm.. ieee80211_check_concurrent_iface() checks for vif.csa_active so I assumed it should be protected by a lock too. But now I'm questioning if it should check csa_active at all? It's just an interface type change. You can't change iftype of a running (i.e. connected/associated/beaconing) interface, right? If interface isn't actually running it doesn't use a channel context, so it is fine to change iftype regardless of CSA state. What should actually be forbidden is to bind to a channel context that is a part of a CSA (which I do in the other patch). local->mtx is also used in the later patch to lock out conflicting (current impl. just assumes concurrent = conflicting) CSA requests. But I wonder (assuming we forege csa_active check in ieee80211_check_concurrent_iface) - if we move CSA to be more chanctx centric it might actually make more sense to depend on chanctx_mtx instead of local->mtx, no? 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
