mii.h: line 431: should just be removed, since you removed this parameter.
mac_provider.h: line 171: Why using __ prefix? Is there some reason the
structure name needs to be hidden from drivers (it seems to me that this
might require additional casting in the framework)?
mac_provider.h: lines 356, 357. You should indicate that these values
are just private, without advertising the guilty party (nxge). Is there
a PSARC contract covering these interfaces, because they seem like they
are contract private instead of consolidation private? (It seems really
unfortunate that we need a special set of defines here for a single
driver. Why are these necessary? I'd consider this a bug.)
mac.h: line 114-115: why did you delete this PSARC reference?
afe.c: LGTM. Although, it might be nice to have a short-cut for the
property capabilities. Say, "MC_PROPERTIES", which includes MC_SETPROP,
MC_GETPROP, and MC_PROPINFO. (This comment applies to all the mac
providers.)
arn_main.c: LGTM
atge.h: Why?
atge_main.c: LGTM
ath_main.c: LGTM
atu.c: LGTM
bfe.c: LGTM. I really wish this were converted to the common MII
framework, but not this case.
bge_impl.h: LGTM
bge_kstats.c: LGTM
bge_main2.c: Line 40. I still think the version number ought to be
removed from this line.
bge_main2.c: Line 152, 153: We really should nuke these private
properties. Not this case, but it probably would have simplified the
changes.
bge_main2.c: line 831: You removed this, but why? Does the framework do
it automatically now?
bge_main2.c: line 994: I'd prefer a _NOTE(ARGUNUSED()) rather than a
blanket /*ARGSUSED*/ Ditto 1073, and anyplace else /*ARGSUSED*/ appears.
bge_recv2.c: LGTM
bge_main.c: LGTM
bridge.c: LGTM
dmfe_main.c: LGTM
e1000g_mac.c: What changed? White space only?
e1000g_main.c: line 49. Another version that I wish we would remove.
e1000g_main.c: line 610. If we're removing the count of private
properties, then how will we iterate for the '?' in ndd (and I have long
wished for a way to discover a driver's private properties as well, via
dladm)? I need to go back and check that code.
e1000g_main.c: line 2862: This change I'm not as comfortable with.
Please have the e1000g maintainer review this. (Ted You?)
e1000g_rx.c: LGTM
e1000g_tx.c: LGTM
e1000g_stat.c: LGTM
e1000g_sw.h: LGTM
hme.c: LGTM
hxge_impl.c: LGTM
hxge_kstats.c: LGTM
hxge_main.c: I need to review this more carefully, but ideally have the
hxge maintainer(s) review it (NSN?)
hxge_rxdma.c: LGTM
hxge_send.c: LGTM
ibd.c: LGTM
ibcm_arp_link.c: LGTM (although a bit surprising that its needed).
igb_gld.c: lines 853, 871: These changes need more careful review.
Please have someone else review them.
igb_rx.c: LGTM
igb_stat.c: LGTM
igb_sw.h: LGTM
igb_tx.c: LGTM
ipw2100.c: LGTM
ipw2200.c: LGTM
iwh.c: LGTM
iwk2.c: LGTM
iwp.c: LGTM
mii.c: lines 716-726: I'd recommend moving these to line 738 (i.e. just
before used, without the intervening case statements).
net80211_ioctl.c: LGTM
zyd.c: line 920, 921. This test is not needed.
mwl.c: LGTM
mxfe.c: LGTM. I wish this one were based on MII, but its harder to
do... some models use a non-MII NWAY scheme.
myri10ge.c: LGTM
myri10ge_lro.c: LGTM
myri10ge_var.h: lines 60-62 -- seems like this driver shouldn't need
netinet/in.h. I'd have tried to remove it. Possibly also the other
netinet/ headers.
nge_main.c: LGTM
nge_rx.c: LGTM
nge_tx.c: LGTM
unm_nic_main.c: LGTM
pcan.c: LGTM. Wish this used the net80211 framework.
pcwl.c: LGTM
rtls.c: LGTM. Warning, I have been planning on a conversion of this
driver to the MII framework. This work *could* conceivably integrate
before b135, and hence before your integration. If so, the merge
changes will be modest (ala afe's change in scope).
sfe_util.c: LGTM
vr.c: LGTM. Please also have Joost Mulders review it. (I'd like to see
this one converted to MII as well, btw.)
yge.c: line 3368. Hmm... perhaps this is better handled via a default:
case in the following switch table?
strsubr.c: LGTM
I'll review the others as I have time.
-- Garrett
Eric Cheng wrote:
> Hi, folks
>
> We'd like to make the code available for review for the following
> additions as part of follow-up to the crossbow project:
>
> PSARC/2009/638 Public GLDv3 Interfaces
> PSARC/2009/501 Dynamic Ring Grouping on NICs
> PSARC/2009/448 pool dladm link property
> PSARC/2009/436 Anti-spoofing Link Protection
> PSARC/2009/364 dlstat and flowstat
>
> Webrev: http://cr.opensolaris.org/~tlc/crossbow_onnv130
>
> The changes also include some bug fixes. The changes/webrev is based on
> build 130. We will update the webrev once in a couple of weeks time to
> include some minor bug fixes, however we'd like to get started on the
> code review now.
>
> Thanks in advance, to Sebastien, Cathy, Girish and Garrett who have
> agreed to review the changes.
>
> Please send comments etc. to crossbow-discuss at opensolaris.org by
> the 15th of Jan. If anyone needs more time, please let us know.
>
> For internal folks, the webrev is @
> /net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev
> /net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev-closed
> The above also has cscope built.
>
> Thanks, and seasons greetings from the crossbow team.
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
>
