Hi Cathy,
Cathy Zhou wrote:
> External webrev:
>
> http://cr.grommit.com/~yun/webrev_uv_09_28
>
> Internal webrev:
>
> http://jurassic.sfbay/net/forwar.prc/builds/yz147064/clearview-uv-review/webrev_review/
>
Here are my comments; nothing earth-shattering. This is overall very good.
usr/src/cmd/dladm/dladm.c
-------------------------
* 2209,2245: It doesn't look like the head and the printed information
quite line up. e.g.:
bash-3.2# dladm show-vlan
LINK VID OVER FLAGS
bge2001 2 bge1 -i---
Testing should run through the output of all sub-commands (including
parseable mode) to make sure everything looks good.
usr/src/cmd/dlmgmtd/dlmgmt_util.c
---------------------------------
* 97,108,122: These functions don't do the finding, but compare two
values. Naming them cmp_link_by_*() would be more appropriate.
usr/src/uts/common/io/dls/dls_link.c
------------------------------------
* 901-906: I thought that this kind of thing would generate lint errors
for non-debug builds, as you end up with an empty block. In any case,
this can also be written as:
ASSERT(dlp->dl_mh == NULL || dlp->dl_mh == mh);
if (dlp->dl_mh == NULL) {
dlp->dl_mh = mh;
dlp->dl_mip = mac_info(mp);
}
usr/src/uts/common/io/dls/dls_mgmt.c
------------------------------------
* 73: Out of curiosity, I'd like to know if there's an inverse of this
operation. In order to allow libdladm in a non-global zone to
operate, it needs a file-descriptor associated with the dlmgmtd door.
An ioctl which returns the fd would do the trick, but I'm not clear on
the details of how to obtain that fd.
* 169: I don't feel comfortable with the lack of a ceiling on the amount
of time we spend attempting (or the number of times we attempt) to do
this upcall.
* 206: The description of the "dev" argument is misleading. I believe
this must only be non-zero for devices under softmac. Is that the
case?
* 210: Correct me if I'm wrong, but no link-id mapping needs to persist
unless something else which is persistent refers to it (like a
persistent property, or the link being part of a persistent
aggregation, or a persistent VLAN being created over the data-link in
question.) If so, then the "persist" argument is an architectural
hiccup, and the logic to determine whether or not to persist a linkid
mapping really belongs in libdladm.
At the same time, information for implicitly created VLANs must never
be persisted, so those semantics are slightly different. In any case,
I'm fine with what you have, but I find it strange that we persist the
link-id mapping for physical data-links when we don't have to.
* 217: I find the "nosupp" argument confusing, but I also noticed that
it was removed from the UV gate a few days ago. Someone else must
have also thought the same. :-)
usr/src/uts/common/io/mac/mac.c
-------------------------------
* 500: s/MACs/MAC/, and the parentheses needs closing in the comment.
* 505-510: This comment block needs some grammatical fixups. We can
work out the specifics offline.
* 547: Checking for the same MAC_CAPAB_PERSTREAM capability on every
open doesn't seem optimal, since you'll be getting the same result
everytime. The open function to call shouldn't change from call to
call, so wouldn't it be better to obtain the open function pointer
once, perhaps during mac_register()? MAC_CAPAB_PERSTREAM isn't
applicable at all unless MAC_CAPAB_PUTNEXT_TX is also supported,
right? If so, it might make sense to simply return the open function
as part of MAC_CAPAB_PUTNEXT_TX and get rid of MAC_CAPAB_PERSTREAM.
* 973: I don't understand the "set" argument. Some intro. comment is
needed to explain it, or alternatively, it should be removed in favor
of some way for the mac module to determine the semantics based on the
state of the mac_impl_t. Snaking up the call chain, I cannot
determine any logic to when B_TRUE is passed in or B_FALSE is passed
in.
Reading how it's used, it would seem to represent the need for the
caller to have us tweak the underlying device's promiscuous mode. I
don't understand how the caller is the authority on this. How does
the caller know what's below the mac module?
* 1551: ASSERT() is too severe here, as it allows a misbehaving driver
to panic the machine, while failing the mac_register() would
gracefully handle the error in a visible way for both debug and
non-debug.
usr/src/uts/common/io/net_dacf/net_dacf.c
-----------------------------------------
* 29: For those not intimatley familiar with the dacf framework, it
would be useful to expand a bit on this comment. For example, explain
which devices these operations apply to. Is it for all DDI_NT_NET
devices? How does the dacf framework know to apply net_config_op for
these devices?
* 62: With the move to Mercurial, we are removing the use of keywords in
the source.
usr/src/uts/common/io/softmac/softmac_dev.c
-------------------------------------------
* 460: Instead of calling dld_wput() here, could we setup q_qinfo in
softmac_open() put's on this queue call dld_wput() instead of
softmac_wput()? I did something like that in iptun (see iptun_dev.c
in the clearview gate).
usr/src/uts/common/io/softmac/softmac_pkt.c
-------------------------------------------
* 42: Why soes softmac need a to define softmac_m_resources() and a
blanking function?
usr/src/uts/common/io/softmac/softmac_main.c
--------------------------------------------
* General: How does softmac_create() end up being called for network
devices which are not supported by softmac and not GLDv3 (such as IB
and PPP for example)?
* 177: An intro. comment is needed here. Given the name of this
function, it's not at all evident that it's called by net_dacf for
_all_ network device nodes. It also performs a different function for
different types of devices (GLDv3 vs. non-GLDv3 for example).
* 215-220: I don't understand this comment. What is its purpose? What
is the impact of sh_cnt not being accurate?
* 256: Could you add a comment explaining why a taskq is needed here? I
can see why a taskq would be needed for calling mac_register(), but
softmac_mac_register() does a whole lot more than that. It's not
clear why all of what softmac_mac_register() does needs to be done by
a taskq. One concern is error semantics for GLDv3 devices. If
dls_create() fails in softmac_mac_register(), for example, shouldn't
we fail the driver's attach?
* 319: I believe this is equivalent to:
*notifications = ~softmac->smac_notifications &
(DL_NOTE_LINK_UP | DL_NOTE_LINK_DOWN | DL_NOTE_SPEED);
* 355: Won't this ASSERT() trigger if (for example) I've named some
existing interface "ce0", and the "ce0" softmac tries to register?
* 368: My impression of softmac_mac_register() is that some code
refactoring is needed to separate the GLDv3 vs. non-GLDv3 code paths,
and perhaps to put some of the code that doesn't have anything to do
with registering with the mac module back into softmac_create(). The
function's name implies that its function is to call mac_register()
for softmac devices, but it does much more than that. Specifically,
could the GLDv3-specific code-paths be up-leveled to softmac_create()?
* 427: Is it really necessary to restrict the use of softmac in this
way? If some 3rd party wants to provide a DL_FDDI MAC-Type plugin,
for example, could softmac work with legacy DLPI FDDI drivers if this
check didn't exist?
* 762: Intro. block comment needed for softmac_mac_recreate().
* 834: Intro. block comment needed for softmac_lower_setup().
* 873: s/nodes/node/
* 879: Where do we do the attach for a style-2 non-VLAN device?
* 960: eri is now a GLDv3 driver.
usr/src/uts/common/sys/softmac_impl.h
-------------------------------------
* This file could stay in usr/src/uts/common/io/softmac/.
-Seb
