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:
signature.asc
Description: Digital signature
