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

[Johannes Berg <johannes@sipsolutions.net>]

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. 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.

> +	chanctx->conf.radar_enabled = radar_enabled;
> +	local->radar_detect_enabled = chanctx->conf.radar_enabled;

What's the reason for "local->radar_detect_enabled"?

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.

> +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?

> +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?

>  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 :)

johannes