On Jan 21, 2008 11:51 AM, Garrett D'Amore <[EMAIL PROTECTED]> wrote: > Code review steps (I'm doing this before even testing the driver): > > First off, by and large, your changes look pretty good to me!
Thanks! That's good to hear considering this is my first pass at this :) > > First (and this is the biggest issue), is that the mac_register > structure is not intended to be long-lived. > GLDv3 is different from GLDv2, in that the intention is that your > private driver state is the "canonical" state you stash with > ddi_set_driver_private(), and the mac_register() is an ephemeral > structure you only use during attach. So in functions like > "find_active_media()" (just to pick one example) take the dnetp as the > argument, instead of the mac_register_t. Additionally, this will make > the code a little leaner, since you're not constantly dereferencing the > mac_register_t. > > Your "soft state" (dnetp) should contain a handle to an opaque > mac_handle_t, which is returned by a pass-by-reference argument to > mac_register. (You already have this as dnetp->mac_handle in your driver. This makes absolute sense. I remember reading in the mac_register() documentation that mac_register_t was a throwaway, but with the prolific use of the older gld struct, I did not want to completely change everything on the first pass. I'll clean this up. > Your dnet_intr() function should take the dnetp as its argument, rather > than the macinfo. I wasn't too sure about the appropriate conversion path for removing gld_intr() - I'll fix this right up. Are there any other gotcha's I should look for by getting rid of gld_intr()? I essentially just replaced calls to gld_intr() to a compatible dnet_intr(). > In dnet_attach, I'd have just removed the error path for kmem_zalloc. > When given KM_SLEEP as its second argument, kmem_alloc and kmem_zalloc > functions are unable to fail. To be honest, I'd like to completely redo dnet_attach() - I think things can be done in a much cleaner fashion (there are 3-4 duplicated blocks for free'ing initialized resources). Expect this to go away. > The additional comments you added to the entry points are a valuable > addition. These were pulled right out of the Nemo design doc. They were really for my own reference, but they seemed useful enough to keep around :) > I'd have removed the DNETTRACE entry code for #ifdef DNETDEBUG... now > that we have DTrace this code just adds complexity that isn't useful. Will do. I'm all for simpler code. > In dnet_getp, you could get more performance by building a linked list > (liked with mp->b_next) of packets to receive, and then passing the > entire chain of them to Nemo. This gives some performance benefit. Its > not a critical change, but worth considering. This makes sense as well; see below. > The other two areas of concern, which may require further investigation, > are support for VLAN tagged full-sized frames (I might be able to help > you out with that), as well as support for link notification. (Old dnet > never had link notification.) The link notification is probably > trickier, largely because you don't always get an interrupt, so you'll > probably have to create a timer, or hang some code off the timer > interrupt on the chip. dmfe has sample code for that (see the cyclic > code, which I think is now called ddi_periodic or somesuch... look for > "periodic" in dmfe.c) I'll probably need a bit of help with this; see below. > I'll try this code out if you want me to, but I'd really like to see at > least the major change to convert away from mac_register before moving > ahead. No need; it sounds like I have a bit of work to do before this is ready for real testing. > If you send me you shipping address, I'll also send you a NIC. I'll send this in a separate message. > -- Garrett below: I think I would like to do this conversion in two phases: (if possible; this is more for first-timer's comfort than anything else). I'd like this first phase to be about converting the driver to GLDv3 in a minimal fashion. Ideally this should expose any number of places where the code can be optimized (ie: dnet_getp()) and where we can gut old entry points, remove obsolete debugging code etc. To me, the goal of this phase is a functioning (but likely not optimal) GLDv3 driver. (removing mac_register_t ref's certainly falls into this category) The second phase should be about optimizations, code cleanup (ie: dnet_attach() initialization), and adding non-esential features (VLAN tagging, link notification etc.) I would also like to completely gut the stats collection and replace it with something more meaningful. Essentially, if this is a driver that I would potentially be responsible for, I would really like to clean it up and modernize it a bit - there is a fair amount of cruft in the driver, and it definitely needs some TLC. Thoughts? Steve _______________________________________________ driver-discuss mailing list driver-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/driver-discuss
