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 ;-)
> /**
> + * 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?
> + 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.
> @@ -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.
> @@ -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
> +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.
> +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?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
