Hi Michal,
thank you for your feedback!
On Tue, Jul 09, 2013 at 09:17:41AM +0200, Michal Kazior wrote:
> Hi Simon,
>
> On 8 July 2013 15:14, Simon Wunderlich
> <simon.wunderlich@xxxxxxxxxxxxxxxxxxxx> wrote:
> > +static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> > + struct cfg80211_csa_settings *params)
> > +{
> > + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > + struct ieee80211_local *local = sdata->local;
> > + struct ieee80211_chanctx_conf *chanctx_conf;
> > + struct ieee80211_chanctx *chanctx;
> > + int err;
> > +
> > + if (!list_empty(&local->roc_list) || local->scanning)
> > + return -EBUSY;
> > +
> > + if (sdata->wdev.cac_started)
> > + return -EBUSY;
> > +
> > + /* don't handle if chanctx is used */
> > + if (local->use_chanctx)
> > + return -EBUSY;
> > +
> > + rcu_read_lock();
> > + chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > + if (!chanctx_conf) {
> > + rcu_read_unlock();
> > + return -EBUSY;
> > + }
> > +
> > + /* don't handle for multi-VIF cases */
> > + chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
> > + if (chanctx->refcount > 1) {
> > + rcu_read_unlock();
> > + return -EBUSY;
> > + }
> > + rcu_read_unlock();
>
> I'm wondering if it's a huge hassle to support drivers that depend on
> channel context API? Perhaps local->open_count could be used instead
> of local->use_chantx and chanctx->refcount?
Actually I'm not using any chanctx drivers, and as long as I can't test
I prefer to keep things disabled. :) local->open_count seems doable though,
it should be the same if there is only one channel context. I can change
that ... If you think it is safe I can enable support, nothing will happen
anyway as long as the driver don't set the "CSA supported" wiphy flag ...
>
> I'm also worried this can possibly do silly things if someone starts
> an interface while CSA is under way. Consider the following:
>
> * start AP
> * initiate channel switch
> [ while channel switch is in progress and driver is yet to call
> ieee80211_csa_finish() ]
> * start another STA interface and associate
> [ CSA completes ]
>
> Upon CSA completion the STA will be moved to a different channel
> silently and most likely end up with a beacon loss quickly. I think
> mac80211 should forbid bringing up any new interfaces during CSA.
I'm afraid you are right about that - there should be some check to
prevent other devices coming up during CSA. I'll add something to
prevent that.
>
> It seems there's nothing preventing from multiple calls to channel
> switch callback. Is this expected? Can this work if CSA is invoked
> while other CSA is still in progress?
This should not happen as long as there is only one interface
doing a channel switch. If this is properly checked (need to fix
the point you mentioned above) that should be safe, I think?
Cheers,
Simon
>
>
> Pozdrawiam / Best regards,
> Michał Kazior.
Attachment:
signature.asc
Description: Digital signature
