Date: Sun, 6 Feb 2011 11:42:43 +0100 From: Bernhard Schmidt <bschmidt@freebsd.org> To: PseudoCylon <moonlightakkiy@yahoo.ca> Cc: freebsd-net@freebsd.org, Alexander Zagrebin <alex@zagrebin.ru> Subject: Re: if_run in hostap mode: issue with stations in the power save mode Message-ID: <201102061142.43865.bschmidt@freebsd.org> In-Reply-To: <147585.89029.qm@web39304.mail.mud.yahoo.com> References: <20110204060808.GA97298@gw.zagrebin.ru> <201102041314.17939.bschmidt@freebsd.org> <147585.89029.qm@web39304.mail.mud.yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 05 February 2011 05:45:59 PseudoCylon wrote: > ----- Original Message ---- > > > From: Bernhard Schmidt <bschmidt@freebsd.org> > > To: Alexander Zagrebin <alex@zagrebin.ru> > > Cc: freebsd-net@freebsd.org; PseudoCylon <moonlightakkiy@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201102061142.43865.bschmidt>