[ Sorry for the delay in responding. ]
> Overall this looks pretty reasonable and extensible for initiating
> ipif/ill quiescence based on async messages from below IP.
Thanks!
> Some minor things below.
>
> * ill_set_phys_addr(): At L23612 ip_if.c if the first copyb fails we
> free up the original 'mp' itself, and we have another free in the
> caller ip_rput_dlpi_writer at L15547 ip.c
I don't think this is the case. The line in question is:
if ((mp = copyb(mp)) == NULL || (mp->b_cont = copyb(mp)) == NULL) {
freemsg(mp);
return (ENOMEM);
}
... but if the first copyb() fails, then it will return NULL and `mp' will
be assigned to NULL prior to entering the body of the `if'. We will then
call `freemsg(NULL)' which is documented to do nothing.
> * ipif_up(): After the call to ipif_ndp_up, we want to return if
> the error is EINPROGRESS, but we fall through.
Yes, this is a bug in my changes (thanks for catching it) -- however ...
> But I am not sure about ipif_ndp_up behavior, it does not seem to
> return the EINPROGRESS I guess something related to Jim's DAD changes?
... it seems that ipif_ndp_up() (even before DAD, according to Jim) has
never returned EINPROGRESS. So the bug above is an accidental fix of
sorts ;-) Still, I will restore original behavior, as I don't want to
modify that logic without a lot more research, and it's out-of-scope for
my changes.
> * ip_reprocess_ioctl() The check at L25359 is redundant. ipsq_current_ipif
> is bound to be NULL here. So we can call ipsq_current_start directly.
> Right ?
This is an interesting case. Is your assertion that ipsq_current_ipif
must be NULL because if it's non-NULL, the it must still have the original
IPSQ value and thus the ipsq_try_enter() in ipif_set_values() will always
trivially succeed (by reentering itself)? If so, I think I agree, but the
current code in Nevada is not so bold -- that is, ip_reprocess_ioctl()
does not ASSERT() that ipsq_current_ipif is NULL prior to setting it to
ill->ill_ipif -- so it ends up working in both cases. The wording of the
comment in ip_reprocess_ioctl() also suggests that there are cases where
ipsq_current_ipif is already set.
For now, I've made ip_reprocess_ioctl() mirror the ipsq_current_start()
code in ipif_set_values() -- but I could be convinced to be more bold.
> One issue that needs some thought below.
>
> Currently (onnv) we try to become exclusive as NEW_OP when we get a
> DL_NOTIFY_IND, the assumption being that this is an asynchronous message
> unrelated to the current operation (if any) in progress. But it is possible
> that drivers may send up DL_NOTE_LINK_UP/DOWN during an interface bring up.
In fact this happens with the ce driver, which I've used during my testing.
> May be there are other NOTEs also that could happen during a bringup
> sequence.
> In the current (onnv) code, such messages would just be queued up and not
> processed till the bring up ioctl completes. May be what is needed is here
> is to actually examine the sub type in the DL_NOTIFY_IND and then determine
> whether it is actually part of the current operation in progress or whether
> it should be treated as something unrelated i.e. as a new operation.
Is there an explicit problem with always treating DL_NOTIFY_IND as a new
operation? Since IP cannot be sure that driver support DL_NOTIFY, it does
not wait for the response for the drivers when it sends the DL_NOTIFY_REQ
probe -- so why would it be appropriate to consider the resulting
DL_NOTIFY_IND's part of the current operation?
Updated webrev:
http://atlantic.east/ws/meem/physaddr/webrev
Thanks again for your help.
--
meem
