by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCHv2 3/6] mac80211: move ibss presp generation in own function

On Fri, 2013-08-09 at 16:35 +0200, Simon Wunderlich wrote:
> Channel Switch will later require to generate beacons without setting
> them immediately. Therefore split the presp generation in an own
> function. Splitting the original very long function might be a good idea
> anyway.

Applied.

johannes

2013-08-16 13:36:36

by Simon Wunderlich

[permalink] [raw]

Subject: Re: [PATCHv2 4/6] mac80211: add support for CSA in IBSS mode

Hey Johannes,

thanks for the comments!

On Fri, Aug 16, 2013 at 12:33:08PM +0200, Johannes Berg wrote:
> On Fri, 2013-08-09 at 16:35 +0200, Simon Wunderlich wrote:
> > Ths
>
> This ;-)
>

yeah ... ;)

>
> > + case NL80211_IFTYPE_ADHOC:
> > + if (!sdata->vif.bss_conf.ibss_joined)
> > + return -EINVAL;
> > +
> > + if (params->chandef.width != sdata->u.ibss.chandef.width)
> > + return -EINVAL;
> > +
> > + switch (params->chandef.width) {
> > + case NL80211_CHAN_WIDTH_40:
> > + if (cfg80211_get_chandef_type(&params->chandef) !=
> > + cfg80211_get_chandef_type(&sdata->u.ibss.chandef))
> > + return -EINVAL;
>
> Is that really correct? It seems that you should be able to switch from
> HT40- to HT40+ and vice versa when switching the channel?

Hmm ... changing HT40+/- can only be represented by using either ECSA (which i did not
implement) or secondary channel offsets in action frames (which comes in a later
patch, but could be merged ...). Secondary channel offsets are not allowed in
beacon/presp, and therefore the client would keep the current mode (HT40+ or HT40-)
as announced in the HT IEs of the beacon/presp. If I add support for secondary channel
offsets in the action frames, the beacons and action frames would contradict, and that
would not be good either.

So I thought it is easier to forbid this case and avoid this mess. :)
>
> And why disallow switching bandwidth (was above this code)? That doesn't
> seem right either?

IEEE 802.11-2012 10.9.8.3 says:

"A 20/40 MHz IBSS cannot be changed to a 20 MHz IBSS, and a 20 MHz IBSS cannot be changed to a 20/40 MHz IBSS."

Also I don't want to allow to switch to a 5 MHz/10 MHz channel or other funky stuff.

> > +/* must hold sdata lock */
>
> pretty useless comment if you have the assert in the function :)
>

Right, I'll remove it. :)

> > + rcu_read_lock();
> > + ies = rcu_dereference(cbss->ies);
> > + tsf = ies->tsf;
> > + rcu_read_unlock();
> > + cfg80211_put_bss(sdata->local->hw.wiphy, cbss);
> > +
> > + old_presp = rcu_dereference_protected(ifibss->presp,
> > + lockdep_is_held(&sdata->wdev.mtx));
> > +
> > + presp = ieee80211_ibss_build_presp(sdata,
> > + sdata->vif.bss_conf.beacon_int,
> > + sdata->vif.bss_conf.basic_rates,
> > + capability, tsf, &ifibss->chandef,
> > + NULL, csa_settings);
>
> This is pretty odd - why does the TSF have to go here? It needs to be
> set by the device when transmitting anyway, no?
>

TBH I don't understand the TSF magic completely, but as far as I know it is
used for IBSS cell merging. What we don't want is to change the tsf when
generating the new beacon and therefore (accidently) kick of some merging process.
Therefore I'm keeping the TSF just as in ieee80211_sta_join_ibss().

ieee80211_ibss_build_presp() needs to put in the beacon, so I need to supply some
valid TSF, don't I?

> > +static void ieee80211_ibss_disconnect(struct ieee80211_sub_if_data *sdata)
> > +{
>
> Is this some refactoring that should be separate? I don't see how it's
> really related to CSA? Maybe I'm missing something?

The only relation is that I need it refactored for IBSS/CSA. Disconnecting for
some reason and going back to search mode wasn't required so far.

I can put that in a separate patchset.

Thanks,
Simon

Attachments:

(No filename) (3.22 kB)
signature.asc (198.00 B)
Digital signature Download all attachments