On Thu, Jan 31, 2013 at 05:46:00PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 17:13 +0100, Simon Wunderlich wrote:
>
> > > > +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > > > + const struct cfg80211_chan_def *c);
> > >
> > > Why does that need to be exported to mac80211/drivers? Shouldn't
> > > cfg80211 be able to check everything?
> > >
> > I'm using it in mac80211/ieee80211_start_ap() to find out whether radar detection
> > is required. We could put into struct cfg80211_ap_settings *params if you prefer?
> > I guess similar params exist for IBSS.
>
> I kinda think that would make more sense because then it's right there
> when you look at the parameters, rather than having to remember it.
>
OK, will change that.
> > > Don't you need a cac_chandef or something?
>
> > The chandef is stored into wdev->preset_chandef anyway, so I didn't see any
> > need to save it twice?
>
> (I'm just worried about that changing, see below. We could use
> "wdev->channel" though, which I need to change to chandef anyway)
>
I'll have a look at wdev->channel then. :)
> > Anyway, the idea was that a driver can report a radar only for a certain part of
> > the currently used spectrum - e.g. we sense a radar on the extension channel in HT40+,
> > we would need to move but could still use the primary channel.
> >
> > I don't know if this is overkill since we don't support any more than HT20 yet, but
> > that would be kind of future proof.
>
> I guess I'm not worried about internal APIs much. Does that even make
> sense? Can you detect radar on one 20 MHz subchannel and then still use
> the other subchannel?
>
According to ETSI, that is possible. To quote from the EN 301 893 v1.7.1:
"If the master device has detected a radar signal on an Operating Channel during In-Service Monitoring, the
master device shall instruct all its associated slave devices to stop transmitting on this channel which becomes
an Unavailable Channel. For devices operating on multiple (adjacent or non-adjacent) Operating Channels
simultaneously, only the Operating Channel containing the frequency on which radar was detected shall
become an Unavailable Channel."
At least in ath9k it appears that the radar header contains some information whether
the radar was received on the primary or extension channel.
I don't know how upcoming 80 MHz devices handle that though.
We can remove it now and re-add it later if you prefer?
> > > > @@ -344,7 +467,10 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
> > > > break;
> > > > case NL80211_IFTYPE_AP:
> > > > case NL80211_IFTYPE_P2P_GO:
> > > > - if (wdev->beacon_interval) {
> > > > + if (wdev->cac_started) {
> > > > + *chan = wdev->preset_chandef.chan;
> > > > + *chanmode = CHAN_MODE_SHARED;
> > >
> > > Ah, ok, so you're using the preset_chandef for CAC as well. I'm not
> > > entirely sure that is correct though, since it could be changed by
> > > userspace without much effect, e.g. by setting the channel with iw or
> > > iwconfig? Unless you added "if (!cac_started)" there everywhere?
> > >
> >
> > Hmm, actually I've tried setting the frequency with iw and got a EINVAL back.
> > I'll look into it again if I missed something, but thought it would be good to
> > not have this stuff redundant.
>
> Ok. Hmm. EINVAL? Maybe you tried setting to a radar frequency or
> something? Can you try setting to say channel 1? I don't think you
> changed __nl80211_set_channel() to check cac_started, so ...
>
I can try again ... but maybe this is obsolete when using wdev->channel.
> > > > + err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
> > > > + if (err < 1)
> > > > + return err;
> > >
> > > That doesn't make sense, if userspace starts CAC and that is successful
> > > it would expect to eventually receive an event that it completed? Thus
> > > if you return 0 here it would get confused, no?
> > >
> >
> > Ah yes, I should probably return EINVAL in this case, or the appropriate
> > error code otherwise ...
>
> Maybe return some more useful error code? Can't really find any one that
> is appropriate though.
We should add EUSELESS. :D
Can't think of anything better, so will keep it at EINVAL until someone has
a better idea.
>
> Do we export the state yet when you do try to get the channel list? That
> would be helpful for userspace, particularly in this case, I think.
>
No, not yet, I've just put that on my TODO.
> > > > + err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, &chandef);
> > > > + if (!err) {
> > > > + wdev->preset_chandef = chandef;
> > > > + wdev->cac_started = true;
> > > > + chandef.chan->dfs_state_entered = jiffies;
> > >
> > > No reason to assign dfs_state_entered since it won't be used in this
> > > state anyway, will it?
> > >
> >
> > Hmm ... yeah, that's stupid. I'll need to add wdev->cac_entered or something like
> > that to track CAC time. Using dfs_state_entered is just wrong.
>
> I didn't see you read that, or did I miss that?
>
chan->dfs_state_entered was read in cfg80211_radar_event() for the CAC_FINISHED event,
which only worked "by accident" because we set the state entered above. :) I'll change
this part too.
> [...]
> > > > @@ -884,6 +884,9 @@ static void handle_channel(struct wiphy *wiphy,
> > > > return;
> > > > }
> > > >
> > > > + chan->dfs_state = IEEE80211_DFS_USABLE;
> > > > + chan->dfs_state_entered = jiffies;
> > >
> > > Here also you don't really need the time assignment.
> > >
> > > (I skipped this before, so pasting here)
> > >
> >
> > Hm, aren't channels initialized in this function? I wanted to set some
> > sane values here - although time is not relevant for the USABLE state,
> > I thought it might be useful if this info is exported to userspace or
> > for debugging.
>
> Maybe so, I just don't think you need the time there since it won't be
> of relevance in the USABLE state. The "state entered" time is only used
> for UNAVAILABLE.
>
> Maybe therefore state_entered should be renamed to "unavailable_until"
> with the corresponding change in the logic of adding the time when it's
> set to that state?
Can do that, if no one is interested in when we, say, change from unavailable
to usable (after NOP). This is what Zefir asked for.
Personally I don't care at all, and we discuss that in the cover letter thread
anyway. Let's see what Zefir says or if anyone else objects, I put that onto
the "TODO if nobody objects list". :)
Thanks!
Simon
Attachment:
signature.asc
Description: Digital signature
