Artem Kachitchkine wrote: > Cathy, > >> 1. Can you think of any non-physical link driver (or even softmac) >> could call mac_prop_init()s? I ask this question because it is >> impossible for pseudo links (aggr, VNIC, or even softmac) to provide >> the instance argument. That number is assigned by the GLDv3 framework. > > This was discussed before and during ARC review and we feel that right > now mac_prop_init/get will be used by physical drivers only. The need > for drivers to actively retrieve properties is currently very limited, > mostly due to some drivers' design defects/peculiarities. Most drivers > should be able to live without these functions, relying on mc_setprop > callbacks only. > If so, please make it clear that this is only used for physical links.
This leads to another two questions: a. Do you expect legacy physical drivers to use this interface, and how? b. Can we remove the second m_instance argument? As I said, it is impossible for a driver to specify it if it is different from the instance number of the dip. >> 2. Assume the code path >> mac_start()->mac_prop_load()->dls_mgmt_linkprop_init()-> >> dlmgmt_upcall_linkprop_init()->dlmgmt_getlink_by_dev() ... >> >> Note that dlmgmt_getlink_by_dev() can only be used for physical links, >> and even for physical links, the device name is not the same as the >> mac name (for example, softmacs). Therefore, when >> dlmgmt_getlink_by_dev() is called with mac_name, it will return ENOENT >> for softmacs and other non-physical links. >> >> The similar problem exists in dld_set_public_prop(). It assumes that >> mac name is the same as the device name. > > Yes, I think this also chimes with Meem's comment about passing linkid > instead of name. It would be cleaner. I need to do some research. One > aspect that bothers me is interaction of dlmgmtd with partially attached > devices - i.e. the attach(9E)->mac_prop_init->mac_prop_load->... chain. > Do you know of any existing kernel functions to translate macnames > into devnames and vice versa? > No, there is no existing functions can do this. Especially if it is before _attach(), when a mac is not even registered, and mac name is not determined yet. >> 3. I assume that you know that mac_prop_init() could fail when a new >> network interface is plugged in or you fresh-install a system. I guess >> since mac_prop_init() is used to apply the persistent link property >> configuration, those failures do not matter. But is a console message >> needed here if that fails (the bge_attach() function, line 3091)? > > I personally don't see the need for console spam here. There's no > messages when ddi_prop_get_int() doesn't find the property, why should > mac_prop_get() be different. I asked Ted to review, maybe he can provide > his opinion also. > Okay. >> 4. I don't see dladm init-linkprop in network/physical service >> anymore. How link properties like autopush can be applied after your >> changes? > > Well, this what this RFE is all about: to remove the need to follow each > ifconfig plumb with dladm init-linkprop. When a link is plumbed, MAC > start will trigger dladm_init_linkprop() and all these properties, > including autopush, will be automatically sent to the kernel. I tested > autopush, among others - it works. > I understand that this RFE is for this purpose. But I have incorrectly thought that datalink autopush must be configured before plumbing. But note mac_start() will be moved to be part of DL_BIND_REQ processing soon (as part of the Clearview softmac fast-path work). In my opinion, mac_start() should have been called as the result of DL_BIND_REQ, as only from then on, the driver is allowed to transfer data. So my question changes to: Is it possible to move mac_prop_load() call from mac_start() to mac_open()? > Now, as Sowmini noted, we still need dladm init-linkprop to push > properties _after plumb_ for wireless drivers. So I'm currently adding > an option to initialize wifi properties only, what you'll see in > network/physical is: > > dladm init-linkprop -w > >> mac.c >> >> - It does not look right to call mac_prop_update() for the ENOTSUP >> case (line 2952). > > Will fix. > Okay. >> - mac_get_prop() does not seem to be symmetric to mac_set_prop(). I >> guess the reason that DLD_PROP_FLAG_MAC_ONLY only applies to >> mac_set_prop() is because in that case, the property is already >> initialized in the underlying driver. But the current flag name is >> misleading because it seems that this property only needs to be known >> by the mac layer. Can we change the name of DLD_PROP_FLAG_MAC_ONLY to >> something like DLD_PROP_FLAG_DRIVER_IS_SET? > > I feel DLD_PROP_FLAG_DRIVER_IS_SET is opaque. Naming is subjective, it's > a matter of perception. Luckily, we don't have to rely on just name to > derive its meaning: we can go to definition and read the comment. I > think it's clear, but I could make it clearer. > Okay. I don't feel strong about it, as long as it can be clearer when people read the code. >> linkprop.c >> >> - In dladm_init_linkprop() function, it does not seem right to take >> (flags | DLADM_OPT_PERSIST) as the last argument. Note the possible >> flags value here is either DLADM_OPT_PROP_MAC_ONLY or >> DLADM_OPT_PROP_MAC_NAME. > > You're right, I should pass the flags via 'argp' argument instead. > Okay. Thanks - Cathy _______________________________________________ networking-discuss mailing list [email protected]
