Attached are comments on the remaining files (I have appended them to the
ones I had sent earlier).
-venu
On Tue, 26 Sep 2006, Peter Memishian wrote:
WiFi for GLDv3 (Nemo) is now ready for code review. This work consists
the administrative, library, and kernel architectural changes necessary to
support WiFi under the Solaris GLDv3 network driver framework.
This is a large project, consisting of almost 15000 new lines of code.
To make the review more manageable, we are splitting it into two phases.
Phase one (which starts now) consists of everything but[1] the net80211
kernel module[2]; we plan to kick off phase two in roughly two weeks.
webrev: http://cr.grommit.com/~meem/wifi-0926
cscope: /net/azariah.prc/builds/meem/wifi-0926/usr/src
/net/azariah.prc/builds/meem/wifi-0926/usr/src/uts
Please provide feedback to this list. The review timer is set for two
weeks (10/10/2006).
Several Sun employees have graciously agreed to do the review (assignments
are listed below), but we welcome comments from the community at-large.
To get oriented, we encourage reviewers to browse the documents tarred up
at http://opensolaris.org/os/community/networking/gldv3-wifi-docs.tar.gz
overview.txt A high-level technical overview of the project,
culled from our PSARC "20 questions" document.
dladm-wifi.pdf An overview of the WiFi administrative model.
gldv3-wifi.txt An overview of the WiFi kernel architecture.
(consolidation private)
lib-wifi.txt An overview of the WiFi libdladm and libwladm APIs.
(consolidation private)
libdladm.txt Specifications for the new libdladm interfaces.
(consolidation private)
libwladm.txt Specifications for the new libwladm interfaces.
(consolidation private)
manpages Revised manpages for dladm(1M) et al.
[ Internal reviewers may also wish to examine the PSARC opinion, full 20
questions, and mail logs for PSARC/2006/406 and PSARC/2006/517 --
though for materials, please use the current ones in the tarball[3]. ]
For this phase, there are 82 files to be reviewed, split into five areas:
1. GLDv3 kernel: 15 files, ~950 lines changed
2. GLDv3 libraries: 5 files, ~3600 lines changed
3. GLDv3 command/misc: 20 files, ~2100 lines changed
4. Atheros driver: 8 files, ~1300 lines changed
5. Packaging/Build[4]: 34 files, ~600 lines changed
In the webrev, each file has been placed into one of the above areas.
My proposed assignments are:
David Edmondson: GLDv3 kernel
Kacheong Poon: GLDv3 library
Renee Danson: GLDv3 command/misc
Venu Iyer: Atheros driver; packaging/build
*Thanks* for your assistance, and let us know if you have questions.
Have fun! :-)
-----
[1] To provide more context when reviewing other kernel files, the net80211
files have been left in webrev/cscope -- but they need not be reviewed
at this point.
[2] For more on net80211, see section 3 of gldv3-wifi.txt.
[3] In particular, there are some small changes between 2006/517 and
the provided materials; we will return to the ARC after code review.
[4] Because our workspace also contains several additional WiFi drivers
that are not part of the initial putback, some packaging/build files
have references to additional drivers not under review. These
references will not be in our final putback.
--
meem
From: http://cr.grommit.com/~meem/wifi-0926
General: Collapse deltas.
usr/src/cmd/dladm/Makefile
L32: Does this mean that aggregation.conf is now going to live in
/etc/dladm. Does it currently reside in /etc? If so would
this cause any issues on existing S10 updates that already
has aggregation.conf in /etc? I know this won't/shouldn't
be hand-edited, but could there be any scripts etc. that
reads this file?
L34: It is taken out because POFILE is in Makefile.master, right?
(I am not sure if
/net/azariah.prc/builds/meem/wifi-0926/usr/src/cmd/dladm is
a built workspace, but I see dladm.po.i, but no dladm.po?)
L40: Owner is dladm?
L61: Just curious is $(ROOTCFGDIR) needed? Should
$(ROOTCFGDIR)/%: $(ROOTCFGDIR) %
$(INS.file)
work? (just checking since I seen something similar in another
Makefile).
usr/src/lib/Makefile
L454: I think this is fine, i.e. explicitly stating the dependency,
but given L485, do we still have to mention libdevinfo here?
usr/src/lib/libdladm/Makefile.com
usr/src/lib/libdladm/common/mapfile-vers
usr/src/lib/libsecdb/help/auths/Makefile
usr/src/lib/libsecdb/help/profiles/Makefile
usr/src/lib/libwladm/Makefile
no comments
usr/src/lib/libwladm/Makefile.com
L38: Do we need to list libsocket and librt in the dependency list
in usr/src/lib/Makefile?
usr/src/lib/libwladm/common/llib-lwladm
usr/src/lib/libwladm/common/mapfile-vers
usr/src/lib/libwladm/i386/Makefile
usr/src/lib/libwladm/sparc/Makefile
usr/src/pkgdefs/SUNW0on/prototype_com
no comments
usr/src/pkgdefs/SUNWckr/prototype_i386
[Haven't read any docs] do we deliver anything for SPARC?
usr/src/pkgdefs/SUNWcnetr/Makefile
no comments
usr/src/pkgdefs/SUNWcnetr/postinstall
Same question as L32 in usr/src/cmd/dladm/Makefile
L29 Can we just remove all the comments? Could we miss some
specific comments because of this?
usr/src/pkgdefs/SUNWcnetr/prototype_com
L50-53 : Not sure why 'dladm' and not 'root' (for owner).
(OK, looking at changes at i.shadow, stdusers.c etc., dladm
is being added for this purpose?)
Shouldn't postinstall be listed as a packaging file "i postinstall"?
usr/src/pkgdefs/SUNWcslr/prototype_com
no comments (don't we deliver 64-bit?)
usr/src/pkgdefs/SUNWcsu/prototype_com
no comments
usr/src/pkgdefs/common_files/i.ftpusers
I am not sure what is being done here (perhaps I should read the
document 8)).
usr/src/pkgdefs/common_files/i.group
not sure what has changed here.
usr/src/pkgdefs/common_files/i.minorperm_i386
L119 Just looking at the changes (and L243) do we need an entry for
"clone:e1000g"
usr/src/pkgdefs/common_files/i.minorperm_sparc
Similar questions as for i.minorperm_i386.
usr/src/pkgdefs/common_files/i.passwd
L116 : Do "15" & "3" have any significance?
L124-124 : This results in adding
"dladm:x:15:3:Datalink Admin:/:" follwoing the enrty
for nuucp? or if nuucp is not present the line gets
appended at the end?
Just curious why nuucp?
usr/src/pkgdefs/common_files/i.shadow
no comments
usr/src/pkgdefs/etc/exception_list_i386
usr/src/pkgdefs/etc/exception_list_sparc
no comments
usr/src/tools/protocmp/stdusers.c
no comments
usr/src/tools/scripts/bfu.sh
L2135 Please add some comments here. So, I think you determine
if this is new dladm (for aggregation.conf) by checking
for /etc/dladm dir.
L6259 Again, should we just ignore all comments?
L6264 Here we don't bother about comments.
usr/src/uts/common/Makefile.files
usr/src/uts/common/Makefile.rules
I suppose your comment about some WiFi drivers not part of the
initial putback affects the entries here, right? I mean do your
comments relate to pcan, pcwl, ipw2100 etc.?
usr/src/uts/intel/Makefile.intel.shared
usr/src/uts/intel/mac_wifi/Makefile
no comments
usr/src/uts/intel/ath/Makefile
just curious why was this dependent on drv/ip earlier?
______________________________________________________________________________
usr/src/uts/common/io/ath/ath_aux.c
L93- Is net80211.h a new file that is being delivered? Can't find it
in onnv, not in this webrev, but see it in usr/src/uts/common/sys
in the workspace. Also, is there a reason these member names in
ieee80211com differ from those in ath_ieee80211.h in onnv?
Since it still seems to be asc_isc in "struct ath".
L183 Add a line after this.
L232 Why is ieee80211channel changed to ieee80211_channel?
L186 Is txq->axq_qnum equal to the value of i'?
L447 Looks like the return value from ath_chan_set gets set
in ic_newstate? If so, what does '1' mean?
L-502-507 Given the change here, is the comment still the same?
L555 Typo uniacst -> unicast
L581 If it should not happen, should we assert?
L563,584,591,595
Given that it returns only 1 or 0 and it is actually
used a a boolean, i.e:
if (!DEV_KEY_ALLOC(ic, key, &keyix, &rxkeyix))
why doesn't cs_key_alloc return a boolean?
L565 Is the continuation spaced correcly? Looks like a tab.
614
L599 This always returns 1, so could cs_key_delete be a void
instead of a int?
L601 I guess we don't really need asc to be defined, we could have
obtained asc_ah directly from ic in L602.
L650-651
L633 The 'and' at the end doesn't seem right.
L641-642 Use "{}" for else.
L648 Add a comment for this.
L696 Since this always returns 0, could ic_reset return void?
Or could a reset fail?
usr/src/uts/common/io/ath/ath_aux.h
usr/src/uts/common/io/ath/ath_hal.h
no comments.
usr/src/uts/common/io/ath/ath_impl.h
L306-310 Why are these macros?
usr/src/uts/common/io/ath/ath_main.c
General: So, is this being nemo-ised? IF so does 54-63 still hold?
L236 Where does 150 come from?
L257 I suppose we can have a NULL here for getcapab?
L577-594 (old code) Is this now taken care in
net80211.c:ieee80211_input(), L3174-?
L637 So is the header now filled via
... mac_wifi_header_cook()..-> mac_wifi_header()? Does the
comment in L645-646 still hold?
L674 (old code) iswep could have been a boolean since it used as
one.
L859-862 Does this comment still hold?
L928 Is cstyle OK with the space before "++"?
L948 Why is error int32_t instead of just int?
L967-975 Does this mean data is not freed, but control is if there
is no xmit buffer?
L1024-1025 When condition spans a line, use {}
L1025 If error != 0, the caller gets the mp?
L1054 Since ath_xmit() doesn't return a boolean, use != 0.
L1216 So, will the scan be set if the state is not IEEE80211_S_SCAN?
Of is the state likely to change after starting a timer?
L1253 Is cstyle OK with the space after '!',
L1417
L1458
L1577
L1444 use ntimer != 0
L1435 It is OK to invoke callback with the lock held?
L1577 There is a macro for this, right?
L1574 This lock released by the caller?
L1580-1581 Use {} for else
L1705 !add should have been enough
L1747 Is this the else for L1738?
L1791 WIFI_STAT_TX_FAILED could have been added @ L1776?
L2019 63 could be a macro.
usr/src/uts/common/io/ath/ath_osdep.c
L159 ath_halfix[i].p != NULL
usr/src/uts/common/io/ath/ath_rate.c
L191 I suppose this isn't needed, (ath_t *)arg in L193 should do.
usr/src/uts/common/io/ath/ath_rate.h
_______________________________________________
networking-discuss mailing list
[email protected]
