Cathy,
Cathy Zhou wrote:
>> 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);
>> }
>>
> Accept.
>
> While I can do what you suggest. Is this (lint error for non-debug
> builds) really matters?
I think if you do a nightly without -D and with -l, you'll see these
errors. I don't think developers run nightly this way since we always
build with -D, and I think lint is only done with the debug build in that
case (that's a guess, I could be wrong).
>
>> 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.
>>
> By looking up the NV code, I don't think there is a function which can
> convert door_handle_t to a fd. But I don't see any problem if dls keeps
> the fd information if needed.
But the specific fd passed in only valid for the process which called the
initial ioctl. There would need to be some way to map that to a
descriptor in a different process making the "I want access to this door"
request...
>> * 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.
>>
> Okay. I will make it also managed by the retry number (so that the total
> maximum retry attempt would be 3), that includes retry attempts caused
> by REVOKED door and EAGAIN errno.
Okay.
>
>> * 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?
>>
> No. It is non-zero for all physical links. Including of those GLDv3
> devices.
Okay, can this be added to the comments?
>
>> * 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.
>>
> Not really. We will need to persist the physical dev_t of a physical
> link when a physical link is created. This physical dev_t is used to
> open the device (and probably force the attachment of this physical
> device so assure the existence of the MAC).
Okay.
>> * 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. :-)
>>
> It is moved from dls_mgmt_create() to a new dls_mgmt_update() function
> (because of bug 6615700.
Okay, so aggr and iptun don't need to deal with this. :-)
>
> If you prefer, I can change this argument to "novanity" to indicate it
> means this physical link does not support vanity naming (e.g., DL_IB
> links).
That's fine too.
>> * 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.
>>
> the MACs of legacy devices (and only them) would have below capabilities:
>
> MAC_CAPAB_TX_LOOPBACK, MAC_CAPAB_PUTNEXT_TX, MAC_CAPAB_PERSTREAM
>
> Do you prefer me to merge all three together - to be something like
> MAC_CAPAB_LEGACY or such.
I'd love to see these merged together. May I suggest MAC_CAPAB_SOFTMAC?
That way, it's clear in the mac code that by asking for this
capability, we're asking, "is this the softmac driver"?
On a related note, since this capability is a private interface between
mac and softmac, you could define the capability's value in mac.h, but
segragate it from the other capabilities in the capability number space.
In other words, create some high-order bit in the enum that's reserved
for private capabilities and put all of the private capabilities in that
number space with a glaring comment stating "these are private to the
softmac module" so that driver developers know to ignore them.
>
>> * 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.
>>
> We cannot remove it.
>
> Because whether "set" is true of false relies on whether the caller is a
> per-stream type DLS clients.
But doesn't the mac module know that all DLS clients are per-stream once
it gets an answer to the MAC_CAPAB_PERSTREAM capability request?
> Note that the per-stream type DLS client
> would have an single lower stream associated to it, and the promiscuous
> mode would be directly through that lower stream. In that case, "set" is
> set to false, and mac_promisc_set() is not responsible to set the
> promiscuous mode of the underlying device (through the shared lower
> stream), neither it causes the update of the MAC's tx callback pointer
> (mi_txinfo and mi_txloopinfo). The only thing mac_promisc_set() does is
> to see whether it needs to call i_mac_notify(mip, MAC_NOTE_DEVPROMISC).
Are you considering splitting off the promiscuous functionality based on
Eric's comments as well?
>
> This helps me to find two problems though:
>
> a. mi_legacy_promisc is not needed. mi_promisc is only incremented and
> decremented when "set" is true in mac_promisc_set(), and that might
> cause th e update of the tx callback pointer.
Okay.
>
> b. mac_promisc_get() should be changed to:
> ...
> if (ptype == MAC_DEVPROMISC)
> return (mip->mi_devpromisc + mip->mi_legacy_devpromisc != 0);
Okay.
>
>> 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?
>>
> See above. I will add some comments.
Great.
>>
>> usr/src/uts/common/io/softmac/softmac_pkt.c
>> -------------------------------------------
>>
>> * 42: Why soes softmac need a to define softmac_m_resources() and a
>> blanking function?
>>
> This is unfortunate, that in order to make use of the POLL capability,
> we have to have those functions.
Oh, right. I forgot about that. Architectural bug in Nemo.
>>
>> 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)?
>>
> I think PPP would be handled in this case. As they don't create
> DDI_NT_NET device node.
>
> But every DDI_NT_NET device (including the IB device) would be
> associated to a softmac_head_t. Then softmac determine whether it needs
> to call mac_register() and dls_create() based on different attribute of
> this MAC. For example, whether it is a native GLDv3 device, and whether
> the media type have a GLDv3 MAC plugin.
Thanks.
>> * 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?
>>
> I've added the comments to explain the reason:
>
> /*
> * Note that this function could be called as a result of a open()
> * system call, and spec_open() already locked the snode (SLOCKED
> * is set). Thefore, we can only start a taskq to finish the rest
> * of work, as the potential ldi_open_by_dev() call would again
> * try to hold the same lock. Before that, inform dlmgmtd that
> * this link is up so that softmac_hold_link() is able to know
> * the correct state of the link.
> */
>
> In general, taskq is only needed for non-GLDv3 drivers.
>
> I will try to restructure this function.
Great.
>
>> * 355: Won't this ASSERT() trigger if (for example) I've named some
>> existing interface "ce0", and the "ce0" softmac tries to register?
>>
> In that case, EEXIST would not be returned. Instead, we will assign a
> "net<N>" name to the ce device.
Okay.
>
>> * 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?
>>
> How to determine the m_type_ident of the mac_register_t? If that can be
> derived from the media type, we can remove this check.
Great question. There is no interface to do this today, but we could
create one. We could have an additional hash table for MAC-Type plugins
hashed on media type, and provide a simply mac module function to lookup
based on media type and return the "ident" of the plugin providing
support for that media type.
>
>> * 879: Where do we do the attach for a style-2 non-VLAN device?
>>
> 878 if (((softmac->smac_style == DL_STYLE2) || (vid != VLAN_ID_NONE)) &&
> 879 ((err = dl_attach(lh, vid * 1000 + softmac->smac_ppa)) != 0)) {
> 880 goto done;
> 881 }
> 882
Heh, it was right in my fact.
>> usr/src/uts/common/sys/softmac_impl.h
>> -------------------------------------
>>
>> * This file could stay in usr/src/uts/common/io/softmac/.
>>
> I could change this. But I'd like to understand the common practice. Why
> dls_impl.h, dld_impl.h are put in usr/src/uts/common/sys?
Good question, and I don't have an answer. Does anyone else have an
answer to this one? I'm okay with following convention, but I don't see
the logical reasoning behind the convention.
-Seb
