From owner-cvs-all@FreeBSD.ORG Sat Sep 17 20:56:00 2005 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2CFE016A41F; Sat, 17 Sep 2005 20:56:00 +0000 (GMT) (envelope-from sam@errno.com) Received: from ebb.errno.com (ebb.errno.com [66.127.85.87]) by mx1.FreeBSD.org (Postfix) with ESMTP id BBA7D43D45; Sat, 17 Sep 2005 20:55:59 +0000 (GMT) (envelope-from sam@errno.com) Received: from [10.0.0.200] ([10.0.0.200]) (authenticated bits=0) by ebb.errno.com (8.12.9/8.12.6) with ESMTP id j8HKtu6j069026 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 17 Sep 2005 13:55:57 -0700 (PDT) (envelope-from sam@errno.com) Message-ID: <432C8486.8060808@errno.com> Date: Sat, 17 Sep 2005 14:03:02 -0700 From: Sam Leffler User-Agent: Mozilla Thunderbird 1.0.2 (X11/20050327) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Damien Bergamini References: <200509171241.j8HCf5ov019561@repoman.freebsd.org> <432C4C6F.80906@errno.com> <00d501c5bbc0$1e8a9240$0300a8c0@COMETE> In-Reply-To: <00d501c5bbc0$1e8a9240$0300a8c0@COMETE> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/iwi if_iwi.c if_iwireg.h if_iwivar.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Sep 2005 20:56:00 -0000 Damien Bergamini wrote: > | You appear to have added a private sta table so that you could have an > | index for each node table entry. However I don't see any place where > | you index into the table; only that you need the index to pass to the > | firmware. This seems to say you only need a per-node index and that can > | be done differently (see below). > | > | FWIW, the intended way to maange per-node driver-private data like this > | is for the driver to extend the node data structure with a private > | definition (override the class decl in c++ parlance) and store the index > | in the private data area. This could eliminate the table and avoid the > | problems you've introduced by holding uncounted references to node data > | structures. It also avoids likely race conditions that'll result from > | having an unlocked data structure that overlaps scope with the net80211 > | node table (I did something similar in the ath driver only to find out > | the hard way it was a mistake). > > No, you missed the point. This is not a table of ieee80211_node's, but > just a table of the neighbours mac addresses. Thus there is no problem > with reference counting, locking and such. > The iwi_find_txnode() function just looks for a mac address in the table > and returns its index (an entry is created if it was not already existing). Sorry, you're correct, these are mac addresses and not node references. But the suggestion still holds. You've got a separate piece of per-node information that logically belongs in a driver-private area of the node. Having it there would eliminate the table lookup; you just take the node pointer and deref to get the index. The intent of driver-private node storage is to eliminate add-on tables like this. > > | This commit msg didn't indicate that you also made changes for WME > | support. You appear to disallow QoS encapsulation of EAPOL frames but > | this is already done in ieee80211_encap. ieee80211_classify should > | assign an AC for all frames so use M_WME_GETAC(m) and don't make > | driver-local decisions unless they are required by the h/w (hard to tell > | from just looking at a diff). > > No. I need to decide on which Tx ring the packet will be sent. > And I must decide this before calling ieee80211_encap() because in case > the ring is full, I want to put the packet back to the if_snd queue. > Unfortunately, the check against EAPOL frames is done in ieee80211_encap(), > not in ieee80211_classify(). If I use M_WME_GETAC(m) and the frame is > an EAPOL frame, I'll end up checking for free space in the wrong ring. Thanks again, that'll teach me to read diffs instead of the final code. ath has no limits on the h/w tx queues, the limits are imposed by the s/w buffers that back each entry so I never needed this. Perhaps we should move the EAPOL check to the classify code as it's required that you classify packets before doing the encap. > > | I suggest that MFC'ing some of these changes so quickly are a bad idea. > > I don't think so. The WME changes have been in 7-CURRENT for a while now > and the new changes are not likely to break anything. Most of the new > code will not be triggered if IBSS mode is not active. > I'd like this diff to make its way in BETA5 so it can be tested by many. > Moreover it fixes the association problems with APs hiding their SSID. > Note that when I MFC'd changes in this driver recently that I did not MFC any of your WME mods. My suggestion was that you not MFC _some_ of the changes; not things like fixing hidden ap handling. re is the final arbiter of what can be MFC'd. Sam