On (10/17/07 19:02), Ted You wrote: > > The following are some initial code review comments and questions for > your ndd changes. I'll continue to study your code changes and will let > you know if I have any further comments.
Ted, thanks for the review.. > (1) bge_main2.c, line 1216. The variable that needs to be modified > should be "param_msi_cnt", not "param_drain_max". good catch; will fix. > (2) bge_main2.c, in function bge_set_priv_prop(), why is "strncmp" > changed to "strcmp"? several people (Artem, Jim) pointed out that strncmp is redundant here; e.g., http://mail.opensolaris.org/pipermail/brussels-dev/2007-October/000460.html given that we are comparing against a const string, I made it a strcmp. > (3) bge_main2.c, in function bge_set_priv_prop(), the new private > properties are: > _drain_max > _msi_cnt > _loop_mode > While old private properties are named as: > _intr-coalesce-blank-time > ... > I think we should use either "_" or "-" to connect the words for > all the private properties to keep the consistency. What's your > thought? good point. For drain_max, msi_cnt, loop_mode, I was using the ndd var name with the "_" prefix (we had a long discussion about having a separate name space for private properties on OpenSolaris and converged on the decision of prefixing them with "_". The details of the discussion are available at the thread: http://mail.opensolaris.org/pipermail/brussels-dev/2007-August/000381.html). so, in the interest of conformance, I can change all the "-" in the other private properties to "_". > (4) bge_ndd.c, line 53 - 64. What does the "orphan" property mean? There are some properties that are exported by bge via ndd, but which seem redundant. See http://mail.opensolaris.org/pipermail/brussels-dev/2007-October/000468.html these params hanve nd_param_t entries that are read-only, but there is no code in the ndd driver (other than the ndd related one) that actually uses this. It's not clear why these params (that I call "orphans" are around). Perhaps you can shed some light, and if these are needed, I can put them back in? > (5) bge_ndd.c, could you please give me some explanation why we make > those ndd changes (using a new data structure instead of the old one)? nd_param_t structure brings too much overhead, once dladm is available, to support these tunables. For example, it makes it hard to examine these values using mdb- one has to to use the indirection through the array: bgep->nd_params[PARAM_ADV_AUTONEG_CAP].ndp_val instead of just storing a 1 bit value for param_adv_autoneg. Also, since the proposal for the ndd compat component is to let dld do most of the nd_getset()-ish code typically duplicated across drivers, the driver no longer needs to track ndp_min, ndp_max etc (most of the MII params are 0/1 values anyway), so I was trying to simplify this. However, that just reminded me of something I forgot: the min/max of param_link_duplex, param_link_speed is incorrect in the invocation for mac_ndd_prop_add()- I will fix this. > (6) bge_kstats.c, I guess this change is to delete the obsolete function > that is not called anywhere, right? correct. there's also a similar fucntion in nge_kstats.c that was missed during PSARC 2007/396.
