Re: [PATCH v2 2/7] ath10k: implement chanctx API

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2015-01-30 at 15:01 +0100, Michal Kazior wrote:

> With `width` you'll change `center_freq1` in many cases. Reading a
> pair of old-and-new of each may not necessarily be fatal but may
> introduce subtle bugs that are hard to debug. Even if the current code
> doesn't really use these values out of mac80211 callback context now
> it may become a problem later.

Yes, this is a good point. Like I said (below), we had a similar issue
with the bss_conf, in our case that was two timestamps there that need
to be read together. Right now we're the only ones using those and only
within the context of the callback, so we can just document that they're
only valid together in such a context, but it's a more general issue.

> > We've recently seen a similar issue with vif->bss_conf, and I'm
> > considering making that an RCU-protected pointer rather than the
> > embedded sub-struct, that way mac80211 can replace it atomically when
> > needed. I've yet to see if that's practical though.
> 
> It'd be lovely if I could freely read mac80211 structures from
> whatever context I want without any worries but I think you'd have to
> introduce explicit locking between mac80211 and drivers or go ham with
> RCU :-)

Yeah, locking there would perhaps not be such a bad idea? Either that or
RCU. Though we need RCU already in many contexts (like a tasklet
iterating the list of interfaces) but perhaps configuration accesses
could be contained to mutex-capable contexts? I don't know.

We did have a similar problem between cfg80211 and mac80211 and decided
to move the cfg80211 mutex to the public part so that mac80211 (or other
cfg80211 drivers, of course) could use it - and in fact the cfg80211 API
requires that now in some places - but we haven't really considered this
so much in the context of drivers.

I think this would make a good discussion topic for the wireless
workshop :)

johannes