Thanks for the comments, please find responses below. We will send out the updated webrev.
thanks, -venu > >----------------------------------------------------------------------------- > >GD1: mii.h: line 431: should just be removed, since you removed this parameter. > ACCEPT >GD2: 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)? We want to keep the corresponding structure opaque to drivers, and still ta ke advantage of type checking. Sure there are some casts needed as a side effect , but only a very few in the framework, and none in the drivers. >GD3: 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.) ACCEPT - I will update the comment. For some background, MAC_RING_RX_ENQUEUE was needed to workaround a nxge bug (6816730 nxge does not build RX packet chains when operating in interrupt mode) that has been fixed since then. I filed 6923555 to track this. There are other issues in the nxge TX path that required us to do serialization on TX, and these will be addressed in the future. > >GD4: mac.h: line 114-115: why did you delete this PSARC reference? > The bulk of that PSARC case is now obsoleted by the new changes. It's not very useful to have only some references here and there that are not updated as the code changes and new cases are defined. It could actually confuse more than help the reader. >GD5: 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.) ACCEPT - I will add a short-cut, but I'm not committing to modify every driver to use that shortcut. > >GD6:arn_main.c: LGTM > >GD7:atge.h: Why? I needed to remove mac_flow.h inclusion from mac_provider.h, atge.h was referring to some definitions present in mac_flow.h. > >GD8:atge_main.c: LGTM > >GD9:ath_main.c: LGTM > >GD10:atu.c: LGTM > >GD11:bfe.c: LGTM. I really wish this were converted to the common MII >framework, but not this case. > >GD12:bge_impl.h: LGTM > >GD13:bge_kstats.c: LGTM > >GD14:bge_main2.c: Line 40. I still think the version number ought to be >removed from this line. ACCEPT - OK, I'll remove it while I'm there. >GD15: bge_main2.c: Line 152, 153: We really should nuke these private >properties. Not this case, but it probably would have simplified the >changes. > I'd rather avoid more changes unless necessary at this point. I would prefer that filed a CR to track it instead. >GD16:bge_main2.c: line 831: You removed this, but why? Does the framework do >it automatically now? Yes. >GD16: bge_main2.c: line 994: I'd prefer a _NOTE(ARGUNUSED()) rather than a >blanket /*ARGSUSED*/ Ditto 1073, and anyplace else /*ARGSUSED*/ appears. ACCEPT - I thought I had used the _NOTE() everywhere, I guess this one fell through the cracks. > >GD17:bge_recv2.c: LGTM > >GD18:bge_main.c: LGTM > >GD19bridge.c: LGTM > >GD20:dmfe_main.c: LGTM > >GD21:e1000g_mac.c: What changed? White space only? > the cast was added >GD22:e1000g_main.c: line 49. Another version that I wish we would remove. > ACCEPT >GD23: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. > The m_priv_props list is now NULL-terminated. >GD24:e1000g_main.c: line 2862: This change I'm not as comfortable with. >Please have the e1000g maintainer review this. (Ted You?) Samuel Tu reviewed these changes as well. > >GD25:e1000g_rx.c: LGTM > >GD26:e1000g_tx.c: LGTM > >GD27:e1000g_stat.c: LGTM > >GD28:e1000g_sw.h: LGTM > >GD29:hme.c: LGTM > >GD30:hxge_impl.c: LGTM > >GD31:hxge_kstats.c: LGTM > >GD32:hxge_main.c: I need to review this more carefully, but ideally have the >hxge maintainer(s) review it (NSN?) Michael Speer who was with NSN until he joined the Crossbow team was one of the main authors and reviewed the changes. We've sent the code review request t o NSN as well. > >GD33:hxge_rxdma.c: LGTM > >GD34:hxge_send.c: LGTM > >GD35:ibd.c: LGTM > >GD36:ibcm_arp_link.c: LGTM (although a bit surprising that its needed). > >GD37:igb_gld.c: lines 853, 871: These changes need more careful review. >Please have someone else review them. > Samuel also reviewed these changes. >GD38:igb_rx.c: LGTM > >GD39:igb_stat.c: LGTM > >GD40:igb_sw.h: LGTM > >GD41:igb_tx.c: LGTM > >GD42:ipw2100.c: LGTM > >GD43:ipw2200.c: LGTM > >GD44:iwh.c: LGTM > >GD45:iwk2.c: LGTM > >GD46:iwp.c: LGTM > >GD47:mii.c: lines 716-726: I'd recommend moving these to line 738 (i.e. just >before used, without the intervening case statements). ACCEPT > >GD48:net80211_ioctl.c: LGTM > >GD49:zyd.c: line 920, 921. This test is not needed. ACCEPT > >GD50:mwl.c: LGTM > >GD51:mxfe.c: LGTM. I wish this one were based on MII, but its harder to >do... some models use a non-MII NWAY scheme. > >GD52:myri10ge.c: LGTM > >GD53:myri10ge_lro.c: LGTM > >GD54: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. ACCEPT - Will try to remove, we will see if they are really needed > >GD55:nge_main.c: LGTM > >GD56:nge_rx.c: LGTM > >GD57:nge_tx.c: LGTM > >GD58:unm_nic_main.c: LGTM > >GD59:pcan.c: LGTM. Wish this used the net80211 framework. > >GD60:pcwl.c: LGTM > >GD61: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). > Thanks for the heads-up. We will merge as needed. >GD62:sfe_util.c: LGTM > >GD63:vr.c: LGTM. Please also have Joost Mulders review it. (I'd like to see >this one converted to MII as well, btw.) He no longer seems to be with Sun. We have sent the code review annoucement on the relevant opensolaris.org aliases, so if he is still following these aliases he had the chance to do the review. > >GD64:yge.c: line 3368. Hmm... perhaps this is better handled via a default: >case in the following switch table? ACCEPT > >GD65:strsubr.c: LGTM >> 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.
