Garrett D'Amore wrote: > I sure do wish you'd generate a real webrev. The problem with your > diffs is that they lack line numbers, which makes it harder to offer up > review comments.
I apologize for the diff (it was generated directly from mercurial). I've had a hell of a time getting webrev to function properly with mercurial. > Anyway, here's some thoughts so far: > > 1) no need to initialize macp->m_pdata or macp->p_data_size. (In fact, > those fields should probably *not* be touched.) Gotcha. nemo-design seemed to indicate these needed to be initialized, (NULL and 0 respectively IIRC) I'll take this out. I'll double check mac_register() to see if there are any other member initializations that should be taken out. > 2) rather than fix the various DNETTRACE cmn_err's, I'd just remove > those blocks --- you're already touching the cmn_err, which isn't > needed, so better to clean it up this time around. (But only the ones > like DNETTRACE that just show function entry or exit points. I've not > checked for other DNETDEBUG cases.) Works for me - I'll cut these out. Would this be an appropriate time to begin adding dtrace probes in place of cmn_err? What is the policy for adding sdt probes to kernel modules? > 3) Just a question: can tx_interrupt_mask in the dnet_mc_tx() routine > just be treated as a constant? I'm not sure with the removal of the > GLD_INTR_WAIT code that there is any point to having a separate variable > here. But I need to look at the whole function to verify that. > > 4) Ditto for the handling in dnet_intr(). I'll look into this as well. If it can be removed, I'll make sure to get rid of it. > The rest of this looks pretty good. Great. I've started doing some testing, there are a few things that still need to be tweaked, but as soon as I have something that has the above changes I'll post another patch (hopefully as a webrev next time). Steve -- Yet magic and hierarchy arise from the same source, and this source has a null pointer. Reference the NULL within NULL, it is the gateway to all wizardry. _______________________________________________ driver-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/driver-discuss
