Re: [PATCH 1/2] cfg80211: initialize rate control after station inserted

[Johannes Berg <johannes@sipsolutions.net>]

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

Hi Reinette,

> This work is motivated by an attempt to untangle the iwlwifi station
> management to be able to use mac80211's sta notify callback. From 4965
> and up the rate scaling in the device is done per station, so an entry
> in the station table is required for the rate scaling initialization to
> succeed. 

Interesting. I've been thinking about making it go the other way --
remove rate scaling hooks completely. wl1271 apparently has rate scaling
completely in the firmware, so the RS algorithm on the host is just
overhead. I've been thinking putting 4965+ RS into the _driver_ makes
more sense since it really does a lot in the firmware and not on the
host.

Do you think we should try to go that route? I'd think it would probably
be a hardware flag ("no RS algo please") and then we'd skip all the
hooks and put things into the driver. The advantage is that we don't
care about the mac80211 API any more, things get cleaner and we can just
do all RS init from sta_notify().

I've also been thinking if there's a way to make sta_notify() able to
sleep but so far I don't see one unfortunately.

Thoughts?

Anyhow, thanks for the explanation.

> > > @@ -742,13 +740,17 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
> > >  		if (err == -EEXIST && layer2_update) {
> > >  			/* Need to update layer 2 devices on reassociation */
> > >  			sta = sta_info_get(local, mac);
> > > -			if (sta)
> > > +			if (sta) {
> > > +				rate_control_rate_init(sta);
> > >  				ieee80211_send_layer2_update(sta);
> > > +			}
> > >  		}
> > 
> > Why is this necessary? It should already have been called for this
> > station earlier?
> 
> maybe - I just tried to have the code behave exactly as before, just
> with the rate scale initialization called later. Even before this patch,
> rate scaling initialization would be called if the station already
> exists. 
> 
> If it is not necessary I can remove it.

No, right, I understand now.

> Right now iwlwifi is adding stations inside the rate scaling code in
> order to work around this issue. I'd like to clean this up and only use
> the sta notify callback.

Makes sense, thanks, I appreciate that -- should be a good cleanup to
the driver and reduce the number of places that try to add a station and
make the driver more streamlined.

> > Same in ibss.c (not quoting it here) where you're only moving it to
> > after sta_info_insert()
> 
> This was my goal actually.

Yeah, I finally understood :)

> >  -- all that seems to do is add race conditions,
> > allowing other code to find not-yet-initialised stations.
> 
> I did not realize that this can happen. Can you please elaborate?

As soon as sta_insert() got called, a packet transmitted to that station
can be processed, find the sta info, and it seems we could end up
calling rate_control_get_rate() before the init was done, through a race
condition.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]