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

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

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

On Wed, Feb 06, 2013 at 06:33:35PM +0100, Johannes Berg wrote:
> On Mon, 2013-02-04 at 13:49 +0100, Simon Wunderlich wrote:
> 
> >   *
> >   * @channel: the channel to tune to
> >   * @channel_type: the channel (HT) type
> > + * @radar_enabled: whether radar detection is enabled on this channel
> 
> There's only one channel for ieee80211_conf ;-)
> 

OK

> >  /**
> > + * ieee80211_radar_detected - inform a configured connection that
> > + * radar was detected on the current channel
> > + *
> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> > + * @gfp: context flags.
> > + */
> > +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp);
> 
> Given the way this works in cfg80211 and my comment there, it seems
> pointless to report per vif, but rather should be per HW?
> 

Yeah ... might be true. userspace can see for itself if it is affected
(for the case we use 40 MHz channel and a 20 MHz user is not interested
in problems on the extension channels)

> > +	res = ieee80211_vif_use_channel(sdata, chandef,
> > +					IEEE80211_CHANCTX_SHARED);
> > +	if (res)
> > +		return -EBUSY;
> 
> return res? This really can't fail here (except for memory allocation
> etc.) so -EBUSY is a bit odd.
> 

OK

> > @@ -753,6 +753,9 @@ struct ieee80211_sub_if_data {
> >  	int user_power_level; /* in dBm */
> >  	int ap_power_level; /* in dBm */
> >  
> > +	bool radar_required;
> > +	struct delayed_work dfs_cac_timer_work;
> 
> Does the work struct make sense here? It seems like an inherently global
> operation, so should that be in ieee80211_local? We should check anyway
> if radar detection is requested when it's already running, I guess.
> 
> > +++ b/net/mac80211/iface.c
> > @@ -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);
> 
> OTOH, I guess if the interface is going away then you'd want to stop
> radar detection if it was done for that interface, so in that sense it
> makes sense per interface.
> 

For the CAC I would like to keep it like this - it is used per interface. For NOP
stuff it must be per wiphy to enable channels later, and it is implemented like that.

> > @@ -1583,6 +1592,9 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
> >  	spin_lock_init(&sdata->cleanup_stations_lock);
> >  	INIT_LIST_HEAD(&sdata->cleanup_stations);
> >  	INIT_WORK(&sdata->cleanup_stations_wk, ieee80211_cleanup_sdata_stas_wk);
> > +	INIT_DELAYED_WORK(&sdata->dfs_cac_timer_work,
> > +			  ieee80211_dfs_cac_timer_work);
> > +
> 
> unneeded blank line
> 
OK
> > +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();
> > +	ieee80211_vif_release_channel(sdata);
> > +	cfg80211_radar_event(sdata->dev, &sdata->vif.bss_conf.chandef,
> > +			     NL80211_RADAR_CAC_FINISHED, GFP_KERNEL);
> > +	rtnl_unlock();
> > +}
> 
> Did you test your code with lockdep enabled? I'm almost certain using
> rtnl_lock() isn't allowed on mac80211's workqueue.
> 

Nope, I didn't test. Will do that.

> > +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp)
> 
> That "gfp" argument is misleading, ...
> 
> > +{
> > +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> > +
> > +	trace_api_radar_detected(sdata);
> > +
> > +	/* may happen to devices which have currently no BSS configured */
> > +	if (!cfg80211_chandef_valid(&sdata->vif.bss_conf.chandef))
> > +		return;
> > +
> > +	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
> 
> ... since you do something that requires being able to sleep, hence the
> only useful argument you could pass as gfp is GFP_KERNEL. However, not
> being able to calls this from softirq or so is probably not desirable
> for many drivers?

Hmm ... yeah that's right. Calling it from softirqs is probably desireable. Maybe
another workqueue or just use cancel_delayed_work() ... I'll think about that.

Thanks,
	Simon

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