Now actual review feedback.
1) It looks like the cmn_err() I was talking about in my last post is
already there. WIth the additions in mac.c 1037-1054, you can remove
1055-1066, I believe.
2) mac.c: lines 2918-2919 are superfluous.
3) mac_ndd.c: Since '?' is normally used only by humans, maybe this
would be a good time to issue a warning inline? Alternatively, just
replacing the '?' with a message to see dladm(1M) may be appropriate.
(But due to i18n concerns, maybe the better way to do this is in ndd(1M)
itself, using a special errno to indicate that the driver is a Brussels
driver?)
4) mac_ndd.c: line 247: Nit: furthermore, this should probably be
strncmp or somesuch. A property name "could" start with '?'.
5) mac_ndd.c: line 255: I don't think you need a check for reading
properties.
6) mac_ndd.c: line 391: But you do need it here. Although its not
ip_config but net_config.
7) mac_ndd.c: ~line 295 "value" and "newvalue" are confusing. Maybe use
"valueptr" to indicate its a pointer? And then s/newvalue/value/.
8) mac_ndd.c: line 322-323: this test (and the relationship to this
loop, and the loop above it) are somewhat confusing. Maybe just nest
the for in an if {} block? Or use a goto in the above loop, putting a
label "prop_found" at 339? (then you could remove the prop_found
boolean variable, and the associated tests
9) should the mapping code include tests for "-" substituted with "_" ?
You called these outliers, but I'm not so sure.
10) mac_ndd.c: mac_mii_prop implies these are only mii properties.
Maybe "mac_public_prop" is a better choice?
11) dld.h: no change obvious
12) The more and more I think about it, the more I dislike the
mac_ndd_handle separation. These should just be Brussels properties.
Period. Having a separate mechanism for drivers to expose different
sets of properties via NDD versus Brussels seems to add complexity and
confusion, and I see no merit in it. That does mean that non-public
properties are only ever interpreted as a string, but that's how NDD
works today. What do you think?
Thanks.
-- Garrett
sowmini.varadhan at sun.com wrote:
> I just put out a preliminary prototype webrev for achieving
> NDD compat through shims in the mac layer at
>
> http://cr.opensolaris.org/~sowmini/nddcompat/
>
> the basic idea is that the ND_SET and ND_GET ioctls will be
> trapped in the mac layer and dispatched as mc_setprop and
> mc_getprop calls.
>
> In the webrev, when the driver calls mac_register(),
> it also registers its NDD tunables (see the call to bge_nd_init()
> in bge_ndd.c) at which time it passes a (void *) pointer with
> information that it can use when called back, to set/get values.
>
> Then, when the ND_SET/ND_GET ioctl is intercepted in the mac
> layer, mac_ndd_{set,get}_ioctl() (in the new file, mac_ndd.c)
> looks up the registered entries for that driver, and if a match
> is found, invokes the mc_setprop/mc_getprop with property number
> set to DLD_PROP_NDD_LEGACY.
>
> There's an additional level of optimization possible, that
> I'm toying with. Since most ndd properties (not counting
> the outliers such as those related to flow-control, or
> those that have - and _ interchanged) are now public properties.
> So, the mac layer can map the ndd name passed with the ND_SET/ND_GET
> ioctl to a public property number, and just invoke the public
> property itself. I've demo-ed this in the webrev for the "adv_autoneg_cap"
> property (leaving out the rest for the sake of readability).
>
> Doing this mapping is cleaner, because a well-behaved driver
> (one that has no outliers) will not need to register anything,
> and can just gut out any ndd code it has today.
>
> But doing this mapping is also a little evil- if I were to write
> a clean driver "foo" today, that only uses Brussels interfaces,
> I would find that I could use both ndd and dladm to set the
> MII props even though I have no intention of supporting ndd
> configuration. In other words, this silently perpetuates the
> usage of ndd where none is intended. So is this serious enough
> to disqualify the ndd <-> "public property" mapping optimization?
>
> --Sowmini
>
> _______________________________________________
> brussels-dev mailing list
> brussels-dev at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/brussels-dev
>
