Re: [PATCHv7 2/3] mac80211: add radar detection command/event

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]

On Thu, Jan 31, 2013 at 03:44:08PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 13:21 +0100, Simon Wunderlich wrote:
> 
> > +void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
> > +				    struct ieee80211_chanctx *chanctx)
> > +{
> > +	struct ieee80211_sub_if_data *sdata;
> > +	bool radar_enabled = false;
> > +
> > +	lockdep_assert_held(&local->chanctx_mtx);
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > +		if (sdata->radar_required)
> > +			radar_enabled = true;
> 
> I guess you could break here. 

yes, OK.

> Technically though I'm not sure this is
> right, since it should only iterate interfaces on this chanctx. OTOH,
> multiple channel contexts aren't (currently?) allowed anyway.

I wonder of multiple channel contexts ever make sense for the DFS case.
If they do, we would have to change this anyway to detect radars on
one channel but (maybe) not on the other ... Anyway, I would leave that
to future strategists. ;)

> 
> > +	chanctx->conf.radar_enabled = radar_enabled;
> > +	local->radar_detect_enabled = chanctx->conf.radar_enabled;
> 
> What's the reason for "local->radar_detect_enabled"?

Interaction checking with ROC and scan.
> 
> Btw, do we also make the driver responsible for turning off any
> powersave when radar detection is enabled? I guess so?
> 
> > @@ -817,6 +817,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
> >  
> >  	cancel_work_sync(&sdata->recalc_smps);
> >  
> > +	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
> > +
> > +	/* only inform about abort cac if it was started before. */
> > +	if (sdata->wdev.cac_started) {
> 
> I'm not entirely sure we should use the wdev data here, OTOH it seems
> safe, so why not.
> 

If you don't mind, it's easier like that ... ;)

> > +void ieee80211_dfs_cac_timer_work(struct work_struct *work)
> > +{
> > +	struct delayed_work *delayed_work =
> > +		container_of(work, struct delayed_work, work);
> > +	struct ieee80211_sub_if_data *sdata =
> > +		container_of(delayed_work, struct ieee80211_sub_if_data,
> > +			     dfs_cac_timer_work);
> > +
> > +	rtnl_lock();
> 
> what part requires rtnl?
> 
ieee80211_vif_release_channel() calls __ieee80211_vif_release_channel()
and has ASSERT_RTNL() before parsing the AP VLAN list.

cfg80211_radar_event() probably doesn't need it ... I should remove it
from the rtnl lock, I guess?

> > +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp)
> > +{
> > +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> > +
> > +	trace_api_radar_detected(sdata);
> > +
> > +	/* may happen to devices which have been added but are not up */
> > +	if (!cfg80211_chandef_valid(&sdata->vif.bss_conf.chandef))
> > +		return;
> 
> Huh, what does device and up have to do with that?
> 

What I've tried:
 * configure 2 SSIDs in hostapd, start it
 * both wlan0 and wlan0-1 got created
 * only wlan0 comes up, wlan0-1 was rejected because of missing channel combinations
 * now I've injected a radar - which should be sent to wlan0 and wlan0-1
 * wlan0 could send the event, but wlan0-1 had no bss configured and therefore no chandef

I can change this comment to "may happen to devices which have currently no BSS configured",
maybe that it is not so confusing ...

> >  static bool ieee80211_can_scan(struct ieee80211_local *local,
> >  			       struct ieee80211_sub_if_data *sdata)
> >  {
> > +	if (local->radar_detect_enabled)
> > +		return false;
> 
> Oh, ok. I guess that explains the reason for radar_detect_enabled :)

Yup. :)

Cheers,
	Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]