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

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

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

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@s2003.tu-chemnitz.de> 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]