From owner-freebsd-net@FreeBSD.ORG Sun Feb 6 19:48:56 2011 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4E61D106566C for ; Sun, 6 Feb 2011 19:48:56 +0000 (UTC) (envelope-from bschmidt@techwires.net) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id DAE228FC13 for ; Sun, 6 Feb 2011 19:48:55 +0000 (UTC) Received: by fxm16 with SMTP id 16so4346301fxm.13 for ; Sun, 06 Feb 2011 11:48:55 -0800 (PST) Received: by 10.223.116.1 with SMTP id k1mr3152660faq.51.1296989158031; Sun, 06 Feb 2011 02:45:58 -0800 (PST) Received: from julie.lab.techwires.net (dslb-088-065-049-167.pools.arcor-ip.net [88.65.49.167]) by mx.google.com with ESMTPS id r24sm812154fax.3.2011.02.06.02.45.55 (version=SSLv3 cipher=RC4-MD5); Sun, 06 Feb 2011 02:45:56 -0800 (PST) Sender: Bernhard Schmidt From: Bernhard Schmidt To: PseudoCylon Date: Sun, 6 Feb 2011 11:42:43 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.1-RELEASE; KDE/4.5.5; amd64; ; ) References: <20110204060808.GA97298@gw.zagrebin.ru> <201102041314.17939.bschmidt@freebsd.org> <147585.89029.qm@web39304.mail.mud.yahoo.com> In-Reply-To: <147585.89029.qm@web39304.mail.mud.yahoo.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201102061142.43865.bschmidt@freebsd.org> Cc: freebsd-net@freebsd.org, Alexander Zagrebin Subject: Re: if_run in hostap mode: issue with stations in the power save mode X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Feb 2011 19:48:56 -0000 On Saturday 05 February 2011 05:45:59 PseudoCylon wrote: > ----- Original Message ---- > > > From: Bernhard Schmidt > > To: Alexander Zagrebin > > Cc: freebsd-net@freebsd.org; PseudoCylon > > 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