Re: [PATCHv2 4/5] mac80211: add channel switch command and beacon callbacks

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:

> + * @channel_switch_beacon: Starts a channel switch for the to a new channel.

for the ?

> + *	Beacons are modified to include CSA or ECSA IEs before calling this
> + *	function. The corresponding count fields in these IEs must be
> + *	decremented, and when they reach zero the driver must call
> + *	ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
> + *	get the csa counter decremented by mac80211, but must check if it is
> + *	zero using ieee80211_csa_is_complete() and then call
> + *	ieee80211_csa_finish().

... when the beacon was transmitted, or something like that, right?

> @@ -2818,6 +2830,8 @@ struct ieee80211_ops {
>  				 struct ieee80211_vif *vif,
>  				 struct inet6_dev *idev);
>  #endif
> +	void (*channel_switch_beacon)(struct ieee80211_hw *hw,
> +				      struct ieee80211_vif *vif);

What about channel contexts? Actually I don't really understand this?
Shouldn't it say which channel to switch to?

>  /**
> + * ieee80211_csa_finish - notify mac80211 about channel switch
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * After a channel switch announcement was scheduled and the counter in this
> + * announcement hit zero, this function must be called by the driver to
> + * notify mac80211 that the channel can be changed.
> + */
> +void ieee80211_csa_finish(struct ieee80211_vif *vif);

If there are multiple interfaces, should it be called multiple times?
etc. Maybe it should be on a channel context instead?

> +/**
> + * ieee80211_csa_is_complete - find out if counters reached zero
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * This function checks the registered beacon if the counters reached zero.
> + */
> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif);

Huh? Return value? Maybe it should be bool?

> +	netif_carrier_off(sdata->dev);
> +	err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
> +					IEEE80211_CHANCTX_SHARED);
> +	netif_carrier_on(sdata->dev);

That seems like a really bad idea, deleting a channel context might tear
down all kinds of device state and might require deleting the interface
first ... I think the chan context API needs to be extended to switch
instead.

> +	if (WARN_ON(err < 0))
> +		return;

This can fail _easily_ too, e.g. if some other vif stays on the channel
and you're now using too many channel contexts.

> +	/* don't handle if chanctx is used */
> +	if (local->use_chanctx)
> +		return -EBUSY;

Still don't really like the way you've implemented it :-)

> +	rcu_read_lock();
> +	mutex_lock(&local->chanctx_mtx);

Yeah right.

> +static inline void
> +drv_channel_switch_beacon(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	if (local->ops->channel_switch_beacon) {

blank line between these two please

> +		trace_drv_channel_switch_beacon(sdata);
> +		local->ops->channel_switch_beacon(&local->hw, &sdata->vif);
> +	}
> +}
> +
> +

steal one from here ;-)

>  void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
> +/* channel switch handling */

I'd prefer a blank line before the comment

> +TRACE_EVENT(drv_channel_switch_beacon,
> +	TP_PROTO(struct ieee80211_sub_if_data *sdata),

Use local_sdata_evt I think.

> +void ieee80211_csa_finish(struct ieee80211_vif *vif)
> +{
> +	struct ieee80211_sub_if_data *sdata = NULL;
> +	sdata = vif_to_sdata(vif);

ahem.

> +	vif->csa_active = 0;

is that a counter, or should it be a bool?

> +static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> +				 struct beacon_data *beacon)
> +{
> +	struct probe_resp *resp;
> +	int counter_beacon = sdata->csa_counter_offset_beacon;
> +	int counter_presp = sdata->csa_counter_offset_presp;
> +
> +	if (WARN_ON(counter_beacon > beacon->tail_len))
> +		return;
> +
> +	if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
> +		return;

How can these happen?

> +	((u8 *)beacon->tail)[counter_beacon]--;
> +
> +	if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
> +		resp = rcu_dereference(sdata->u.ap.probe_resp);

Who guarantees RCU protection?

> +		if (WARN_ON(!resp))
> +			return;

That can legimitately happen, no? At least userspace is allowed to not
set probe_resp now, if you want to change that ...

> +		if (WARN_ON(counter_presp > resp->len))
> +			return;

?
I guess counter_presp should be called "counter_offset_presp" or so

> +		((u8 *)resp->data)[counter_presp]--;

resp->data is already a u8 *, no?


> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif)
> +{
> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +	struct beacon_data *beacon = NULL;
> +	int counter_beacon = sdata->csa_counter_offset_beacon;
> +
> +	if (!sdata || !ieee80211_sdata_running(sdata))
> +		return 0;

yeah right ... container_of returned NULL?

> +	if (vif->type == NL80211_IFTYPE_AP) {
> +		struct ieee80211_if_ap *ap = &sdata->u.ap;
> +		beacon = rcu_dereference(ap->beacon);
> +

swap those last two lines :)

johannes