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

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

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

Hey Johannes,

I'm skipping the "style" comments (you were right most of the time anyway) and will
only comment on the design stuff, see below:

On Tue, Jun 18, 2013 at 05:00:28PM +0200, Johannes Berg wrote:
> On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> > @@ -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?
> 

My first implementation (ath9k) does rely on mac80211 to complete the
channel switch, so it does not even need to know which channel is switched
to. I can add that as we can assume other drivers will behave differently ...

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

Multiple interfaces are not supported - and I don't know how this should be
handled anyway. CSAs are triggered on a per-interface base from userspace,
and multiple CSAs would clash with each other (could be different channels,
different counters, etc ...).

Or would you have a suggestion how to handle this differently?

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

Hm, yeah I can do that.

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

Why not? :)

> > +	vif->csa_active = 0;
> 
> is that a counter, or should it be a bool?

It should be 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?
> 

Maybe when the beacon is re-assigned - although add a check for that
and remove these warnings ...

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

Hmm ... should add that.

> > +		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 there is no presp then also counter_presp should not be set.

Cheers,
	Simon

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