if_run in hostap mode: issue with stations in the power save mode

[Bernhard Schmidt]

On Saturday 05 February 2011 05:45:59 PseudoCylon wrote:
> ----- Original Message ----
> 
> > From: Bernhard Schmidt <bschmidt at freebsd.org>
> > To: Alexander Zagrebin <alex at zagrebin.ru>
> > Cc: freebsd-net at freebsd.org; PseudoCylon <moonlightakkiy at yahoo.ca>
> > Sent: Fri, February 4, 2011 5:14:17 AM
> > Subject: Re: if_run in hostap mode: issue with stations in the
> > power save mode
> > 
> > On Friday 04 February 2011 12:40:21 Alexander Zagrebin wrote:
> > >  Hi!
> > > 
> > > On 04.02.2011 09:51:34 +0100, Bernhard Schmidt  wrote:
> > > > On Friday 04 February 2011 07:08:08 Alexander Zagrebin  wrote:
> > > > > I'm using an Ralink RT2870 based adapter (run(4)  driver) in
> > > > > the hostap mode. and I've noticed that if_run  doesn't
> > > > > support stations working in the power save mode (PSM).  The
> > > > > reason is in lack of the TIM in beacons. The attached  patch
> > > > > adds this functionality and completely fixes this issue. 
> > > > > Despite the fact that patch is working, it seems that it
> > > > > needs  an additional work. For example, now the result of 
> > > > > ieee80211_beacon_update is ignored with a corresponding 
> > > > > message, but may be necessary to process it...
> > > > > 
> > > > > Can somebody review it?
> > > > 
> > > > That  looks about right, good catch!
> > > > 
> > > > Handling  ieee80211_beacon_update()'s return value doesn't seem
> > > > to be  necessary, the mbuf's length is handled in the next few
> > > > lines of  code anyways, doesn't matter if it changed or not.
> > > > 
> > > >  Though, I have a some doubts about just restoring bo_flags is
> > > >  enough (Can't prove that with some obvious code, still..). It
> > > > 
> > > > feels  saner to me if we just reuse the whole mbuf, similar to
> > > > what ath(4)  does. Can you look at attached patch? Completely
> > > > untested, so I'm  not sure what does happen on e.g. changing
> > > > the SSID.
> > > 
> > > I've  thought about such solution, and it looks more right, but
> > > I've decided  just to add ieee80211_beacon_update() to make the
> > > patch clear. I'll try  your patch a bit later, but I already
> > > have a question: on the first  invocation of the
> > > run_update_beacon_cb() only ieee80211_beacon_alloc()  will be
> > > called. So dynamic beacon contents will not updated. Is it a 
> > > problem?
> > 
> > I don't think it is. The work beacon_update does is handling 
> > changes to bo_flags, which are only changed through calls to 
> > iv_update_beacon(), so this is safe, because the driver itself
> > does change  bo_flags which is immediately followed by the beacon
> > update  process.
> 
> I like the way mwl(4) handles it.
> http://fxr.watson.org/fxr/source/dev/mwl/if_mwl.c#L1927
> though I don't know why it uses ieee80211_beacon_alloc() instead of
> _update()
> 
> @@run_update_beacon(struct ieee80211vap *vap, int item)
>  struct run_vap *rvp = RUN_VAP(vap);
> +int mcast = 0;
>  uint32_t i;
> 
> +KASSERT(vap != NULL, ("no beacon"));
> +
> +switch (item) {
> +case IEEE80211_BEACON_ERP:
> +run_updateslot(ic->ic_ifp);
> +break;
> +case IEEE80211_BEACON_HTINFO:
> +run_updateprot(ic);
> +break;
> +case IEEE80211_BEACON_TIM:
> +mcast = 1;/*TODO*/
> +break;
> +default:
> +        break;
>  }
> 
> +setbit(rvp->bo.bo_flags, item);
> +ieee80211_beacon_update(vap->iv_bss, &rvp->bo, rvp->bm, mcast);
> +
>  i = RUN_CMDQ_GET(&sc->cmdq_store);
>  DPRINTF("cmdq_store=%d\n", i);
>  sc->cmdq[i].func = run_update_beacon_cb;

That looks good, it handles the case about slottime changes i've 
mentioned.

> It's been working fine updating protection mode in HT mode past a few
> days. (Some how, iwn(4) works fine with run(4), I cannot tell it
> works fin with power saving mode.)

Afaik iwn(4) doesn't use PS, never got around implementing that.

> I'd like to move ieee80211_beacon_alloc() into iv_vap_alloc(). Then
> we don't need to test beacon_mbuf == NULL in run_update_beacon_cb(),
> and there is already switch we can use for conditionally alloc mem.

Sounds fine with we.

> > There is one expection I'm not sure about how to handle though,
> > slottime. This value might change based on nodes associating and
> > leaving, resulting in a call to ic_updateslot() which is currently
> > commentted out.
> 
> That's only because of LOR. I'm adding another call back function
> since run_updateprot() need to be deferred when it is called
> from  run_update_beacon().

Great, thanks.

Can I talk you into integrating that into Alexander's patch?

-- 
Bernhard