sowmini.varadhan at sun.com wrote: > Garrett, > > thanks much for the review. > > On (03/19/08 10:51), Garrett D'Amore wrote: > >> usr/src/uts/common/io/bge/bge_main2.c >> -------------- >> line 37: Since you're here anyway, can you remove the v0.60 (or %I%) >> from the modinfo output. There is little or no value in having it here, >> and we need to remove kernel ident strings for mercurial. >> > > will do. > > >> line 1082: You removed the check for the valsize. Why? The other >> properties still seem to have these checks. >> > > mistake on my part: this also needs the check. I'll put it back. > > >> line 1088: (And others.) It appears that you're using bcopy, presumably >> to avoid possible alignment problems. Is this necessary? I'm actually >> thinking that for "well-known" properties, the size and alignment checks >> may not be necessary. Of course, this depends on whether someone can >> arrange for an incorrect getprop() to happen from user space. The code >> would be simpler if it didn't have these checks in it, for sure (and >> direct assignment is simpler than bcopy). >> > > I do the bcopy for alignment, as you guessed. I'd prefer to > leave it there because the structure layout can change in > the future, and it's safer to just do the bcopy. (Also suppresses > lint errors cleanly). > > >> line 1266-1287; Is this here just to demonstrate private properties? I'd >> rather have something meaningful here than a synonym for another >> property. (The _drain_max property demonstrates this.) Creating synonyms >> for OpenSolaris seems a bit broken, if we can help it. >> >> line 1355-1372: Same issue. >> > > Actually it is there for legacy support of attempts to get adv_pause_cap > and adv_asym_pause_cap via ndd. >
Are those "well-known" properties? (I.e. do more drivers than just bge make use of those names?) If so, then you should just remap it in the framework (basically silently convert it to the equivalent alias). If this is only for this driver, then I'd nuke it. Also, I'll note that tuning of these values is very rarely done. I doubt anyone really needs these outliers, as long as the "well-known" equivalent names are also available. Going through this particular trouble to support these outliers for just one driver seems kind of wasteful. > >> line 1380, 1382, 1388, 1390, 1396, 1399, 1406, 1408: Here you are using >> sprintf rather than snprintf. But at lines 1357 and 1359 you are using >> snprintf and printing directly into the pr_val. I think what I'd like to >> see is a construct more like this: >> > > I'll make them all use snprintf. > > >> Line 3448-3455: Here you are registering the outliers, which should be >> well-known properties. Yech. Remove if you can. But you should register >> the names for the *other* properties. (I guess maybe you don't want to >> expose them via NDD? >> > > Correct. > More on this below. > >> But I would prefer to see *every* private property >> registered. Otherwise you're using this as an NDD-only API, when I don't >> think that's what we talked about it being earlier. (A generic name is >> pointless if not all private properties are registered with it.) >> > > I guess my take is that we don't want to add more ways to use ndd, > but would prefer to wean people over to dladm as much as possible. > Sure, I understand and agree with that goal. But I also think that we want to register all properties -- dladm may someday want to iterate over them, and that is why I originally requested the name change. Frankly, as I've indicated already, I really, *really* think we could have just done away with all support for private properties in NDD. NDD should only be used to access well-known properties, because all the other outliers are frankly not going to be coded for in scripts, etc. *Human* users need to learn to use dladm. :-) So, in retrospect (I think I suggested this earlier), I'd actually welcome a change that did: 1) offered only well-known names via NDD 2) make all other private properties only available via dladm 3) as a consequence of #1 above, you could actually skip the separate registration of property names for now, until another consumer of them (dladm?) exists Something to think about, at least. - Garrett > >> usr/src/uts/common/io/bge/bge_ndd.c >> >> Line 332: I think you could remove this debug line. That's what dtrace >> is for, right? >> > > Ok. > > >> General: There is so little left here, I wonder if it wouldn't be >> simpler to just move the remainder into bge_main2.c? >> > > I think there is some use to leaving it there as bge_main2.c is pretty > large already. > > >> usr/src/uts/common/io/mac/mac.c >> >> Line 1059, 1064: Hmm... I might have left this in a non-debug kernel. >> Was this specifically requested by someone? >> > > Yes: > http://mail.opensolaris.org/pipermail/brussels-dev/2008-February/000706.html > > (See also CR 6625930) > > >> usr/src/uts/common/io/mac/mac_ndd.c >> >> Line 202: I thought each line needed a null-terminating byte on it, but >> you have added a space. Does this actually work properly? It is not the >> same protocol that I am using in my ndd code in afe. >> > > yes, it works.. I'll check into what happens without the " " > but I seem to recall that /sbin/ndd was getting thrown off in its parsing. > > >> Line 365-394: I'd have structured this so that the kstat case was next >> to the test... i.e. make the test at 365 positive, rather than negative: >> if (mac_ndd_mapping[i].mp_flags & MAC_MAP_KSTAT) { ... }. (Its only a >> nit, but it would help readability. >> > > Ok, will re-arrange. > > >> Line 534-535: You're not holding a lock here. What guarantees do you >> have that you're single threaded? If mac_register_priv_prop() is called >> after mac_register() (which seems to be the case for bge), then you wind >> up with a situation where you might be multithreaded here. Perhaps a >> lock around this link list manipulation would be safer? >> > > (stay tuned, I will shorty be starting a separate thread on this) > > >> usr/src/uts/common/sys/dld.h >> >> Line 50: Please remove the %I% version tag... these need to be >> eliminated in order to support mercurial, and when you're touching the >> driver anyway, its a good time to do this. >> usr/src/uts/common/sys/mac.h >> >> Line 48: Ditto -- remove the %I%. (This definition should probably have >> been in mac_impl.h, anyway.) >> usr/src/uts/common/sys/mac_impl.h >> > > Ok to all of the above. > > >> usr/src/uts/intel/bge/Makefile >> >> Line 73: This comment is no longer true. (Remove "& IP") >> >> usr/src/uts/sparc/bge/Makefile >> >> Line 70: Same comment fix needed. >> > > Ok. > > >> -- Garrett >> >> Sowmini.Varadhan at Sun.COM wrote: >> >>> Garrett, >>> >>> Although you looked at the prototype earlier, could you >>> also please take a quick look at the webrev below? >>> >>> (I have to merge to the nge putback but I'll get the RE for nge >>> to review that component- changes should be easily comparable to >>> bge, I believe) >>> >>> --Sowmini >>> >>> ----- Forwarded message from Sowmini.Varadhan at Sun.COM ----- >>> 1081 case DLD_PROP_SPEED: >>> 1082 speed = bgep->param_link_speed * 1000000ull; >>> 1083 bcopy(&speed, pr_val, sizeof (speed)); >>> >>> >>> >>>>> Extnernal webrev: >>>>> http://cr.opensolaris.org/~sowmini/nddcompat_review/ >>>>> Internal webrev: >>>>> http://zhadum.east/export/ws/sowmini/brussels/nddcompat_review >>>>> >>>>> cscopes: >>>>> >>>>> /net/zhadum.east/export/ws/sowmini/brussels/nddcompat-review/usr/src[/uts] >>>>> >>>>> >>>>> >>>>> Ideally I would like all the reviewers to review all the files >>>>> but if you have time constraints, please minimally send feedback >>>>> on the file list below: >>>>> >>>>> Cathy: >>>>> - usr/src/cmd/dladm/dladm.c >>>>> - usr/src/lib/libdladm/common/linkprop.c >>>>> - usr/src/uts/common/io/dld/dld_drv.c >>>>> - usr/src/uts/common/io/mac/mac.c >>>>> - usr/src/uts/common/io/bge/bge_main2.c >>>>> >>>>> Garrett/Meem: >>>>> - usr/src/uts/common/Makefile.files >>>>> - usr/src/uts/common/io/dld/dld_drv.c >>>>> - usr/src/uts/common/io/mac/mac.c >>>>> - usr/src/uts/common/io/mac/mac_ndd.c >>>>> - usr/src/uts/common/sys/dld.h >>>>> - usr/src/uts/common/sys/mac.h >>>>> - usr/src/uts/common/sys/mac_impl.h >>>>> - usr/src/uts/intel/bge/Makefile >>>>> - usr/src/uts/sparc/bge/Makefile >>>>> >>>>> Garrett/Ted/Crisson/Miles >>>>> - usr/src/uts/common/io/bge/bge_impl.h >>>>> - usr/src/uts/common/io/bge/bge_main2.c >>>>> - usr/src/uts/common/io/bge/bge_ndd.c >>>>> - usr/src/uts/intel/bge/Makefile >>>>> - usr/src/uts/sparc/bge/Makefile >>>>> >>>>> >>>>> >>> ----- End forwarded message ----- >>> >>> > _______________________________________________ > brussels-dev mailing list > brussels-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/brussels-dev >
