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

[PseudoCylon]

----- 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;

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.)

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.

> 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().


AK