On Feb 5, 2010, at 5:16 PM, Garrett D'Amore wrote: > On 02/ 5/10 03:20 PM, venugopal iyer wrote: >> >> >> 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. > > You can have struct foobar as a forward declaration and not provide the > actual details of struct foobar... You don't need to introduce a middle > structure for this purpose.
The new handle is declared completely independently of the underlying structure, and consistently with existing opaque handles defined by mac_provider.h. It's also consistent with the declaration of other DDI opaque handles. I don't see how a handful of type casts in the framework would be an issue. Nicolas. > >> >> >>> 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. > > Ok. > >> >>> 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. > > Fair enough! :-) > >> >>> >>> 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. > > Ok. > >> >>> >>> 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. > > Ok, can you please file one then? > >> >>> 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 > > Thank you. > >> >>> 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. > > Ah, ok. Thanks. > >> >>> 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. > > Good! > >> >>> >>> 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. > > Sounds good! > >> >>> >>> 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. > > Good! > >> >>> 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 > > Thanks! > >> >>> >>> 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. > > And it has so occurred. :-) > >> >>> 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. > > Oh, that's news to me. Ok, well then I'll do the conversion of it to MII > after b134. > >> >>> >>> 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 > > Thanks! > > - Garrett >> >>>> 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. > > _______________________________________________ > crossbow-discuss mailing list > crossbow-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss -- Nicolas Droux - Solaris Kernel Networking - Sun Microsystems, Inc. nicolas.droux at sun.com - http://blogs.sun.com/droux
