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:
signature.asc
Description: Digital signature
