Thanks for the review!
Updated webrev is http://cr.opensolaris.org/~joostm/onnv-vr/
Comments inline.
Thank you, Joost
Garrett D'Amore wrote:
Generally, your code is clean and well written. Thank you.
In your copyright message, it says "joost ad xxx" (instead of "at")
for the e-mail address.
I think you may run into difficulty getting this integrated with
your own copyright... I've found that Sun legal doesn't really grok
the idea that you can integrate stuff into Solaris that is your own
IP. (I had to add a Sun copyright when I fixed bugs in afe or mxfe,
for example. Even though I did it on my own time and the code
originally was written before I was employed at Sun and afe was
explicitly listed in my employment agreement as one of my "prior
inventions".)
This will be a concern for your RTI advocate and C-Team.... but just
wanted to make you aware of it.
(Basically, you can't do any work on "Solaris" on your own time....
apparently it overlaps with Sun business interests and as a Sun
employee your contributions to OpenSolaris are not your own, even if
done "on your own time" with your own resources.)
OK. Thanks for the tip. All files are CDDL only now.
ine 57, 58, 59: You're including some questionable header files:
You shouldn't need anything under inet/ (not DDI compliant), and
even sys/vlan might be questionable. sys/mac_flow.h looks a bit
questionable, unless your driver supports high-end crossbow features
(which I think it doesn't.)
These includes were indeed unneeded. A leftover from the ndd era :-).
The one not removed is sys/vlan.h as this is for de definition of
VLAN_TAGSZ.
Line 309: kstat create can fail reasonably... for example when
adding a new stat if you're using persistent kstats. You probably
shouldn't make this fatal, but then you'd have to check the kstats
pointer is non-null in other places. (E.g. vr_remove_kstats at line
3589 would get a new "if (vrp->ksp)" check. IMO, you should be able
to gracefully deal with kstat initialization failing, without
disabling the function of the entire device.
Done.
Line 576-610: Your method register mapping is rather quaint. You
shouldn't need to examine the "reg" property, but instead use
ddi_regs_map_setup() and friends. (You can inquire about number of
register sets and sizing information with ddi_dev_nregs(),
ddi_dev_regsize(), etc.) I'd rather see you use the DDI convenience
routines here. (Note that config space is set up with
pci_config_setup().)
I agree in that is quaint... but... Memory space accesses don't work
for the old VT86C100A cards. (They have memory space in their bar's
but nothing happens if you write to them via memory space.).
Thus, the driver needs to associate a register set with it's access
method in order to select the "right" one for that type of card. vr
uses memory space by default and io space for the old cards.
According to:
http://developers.sun.com/solaris/developer/support/driver/wps/pci/html/Device_Addr.doc.html
the "reg" way is the only way to do this.
If there is an alternative, I'll be happy to remove it. If it's really
in the way: The driver can try regset 1 and if it doesn't work, try
the next until it works ..
Line 640: Historically bcopy is preferred over memcpy in the
kernel. (Minor stylistic nit.) (IIRC, memcpy wasn't available in
the kernel until S10.)
Done. bcopy replaced by memcpy. On the same line, bzero became memset.
Lines 717-760: I believe you shouldn't need to use driver.conf...
Brussels is the "new" way forward, and I believe that a driver
without legacy concerns shouldn't be using driver.conf anymore.
I added it for consistancy with the current state of solaris' drivers.
Just tell me again it needs to go and it's gone.
Line 944: minor language nit..."out of this list" not "this lists".
Thanks
Line 1202: Is DDI_DMA_STREAMING and DDI_DMA_RDWR appropriate here?
Usually you want streaming only with large allocations, and with
unidirectional transfer. DDI_DMA_CONSISTENT (which will usually use
non-cachable memory) may be better. At the very least you should
figure out the direction and pass only DDI_DMA_READ or
DDI_DMA_WRITE. (Dual direction is appropriate for descriptor
mappings, but not for buffer mappings.)
Done. The bind_handle flags are now passed as an argument and the
caller specifies the direction. There was also a bug in that
DDI_DMA_RDWR was passed to ddi_dma_mem_alloc().
I assume DDI_DMA_STREAMING is good for full ethernet frames. No?
Line 1203: DDI_DMA_DONTWAIT might be suspect... if you're in a
context where sleeping is OK (e.g. only ever called during
attach(9e) time) you can simplify error handling by using
DDI_DMA_SLEEP. This applies to line 1223 as well. (Hmmm, but
you're using dynamic binding rather than a fixed buffer as well, so
maybe you need this... more on that shortly.)
DDI_DMA_DONTWAIT is used because vr_alloc_dmabuf() is used in the
receive path driven by vr_intr(). If vr_alloc_dmabuf() fails during
receive, a new message block is created and passed upstream instead.
Line 1287: I see you're using desballoc. For a 100M driver, I think
this adds complexity that is questionable.
For modern CPU's, the extra complexity of "zero copy" for a 100M
device does not outweigh the advantage of an easy maintainable driver.
However, I specifically wrote this driver for use on my 600Mhz VIA C3,
(http://joostm.nl/solaris/onix/), and on this box, it matters. Every
cycle not spend on copying data, is used for something useful.
Paritcularly, you need to
make sure that you don't have a race with your driver getting
unloaded (e.g. via modunload()) while you still have mblks that are
"loaned up". I believe your driver suffers from this race, which
could ultimately lead to a panic in certain situations. "Fixing"
this requires the driver to use a lock to guard modunload() from
completing successfully if there are outstanding mblks via
desballoc. (That also means you need a global reference counter.)
Again, for a typical 100Mbit driver, the perf. gain is questionable
anyway, so I'd not bother with this and just use bcopy. (Plus bcopy
solves a lot of extra problems with resource management...)
This is in detach:
418 if (vrp->dmamem > 0) {
419 vr_log(vrp, CE_NOTE, "Can't detach, buffers in upper layer.");
420 return (DDI_FAILURE);
421 }
vrp->dmamem is the amount of memory allocated for DMA buffers for that
instance. Is this sufficient?
This would solve the problem described at line 1377 as well, because
you can then bcopy into a misaligned buffer, avoiding an extra data
copy in the IP stack later.
If you don't change this otherwise, you must add safeguards against
the modunload race, and you *should* conditionalize it on the length
of the packet. (Tiny packets will get dismal perf. on this path.)
The TX path is changed so that only packets of 128 bytes and above,
are mapped.
Line 1680: This conflicts with the information in the man page
indicating the card doesn't transmit flow control frames.
The man page states
LIMITATIONS
The vr driver does not support asymmetric flowcontrol.
VT86C100A and Rhine II adapters are not capable of transmitting
flowcontrol messages.
Only the "older" cards can't send pause frames. Where do you see a
conflict?
Line 1786. Similar to the rx comments above, this approach, which
is common but IMO ugly, is better eliminated and using a bcopy of
the frame into prealloc'd frames. Especially, DMA setup and tear
down is expensive and this actually *hurts* performance worse than
just using bcopy on *many* platforms... especially if the frames are
not full MTU frames. I think I wouldn't bother with this extra
effort, but go with the simple bcopy approach. You've got to do
that anyway if the packet doesn't fit your alignment or block count
requirements, so its simpler to just skip all the tests and do the
copy unconditionally, IMO. (For larger frames -- say more than 512
bits, the tradeoff may favor DMA setup and teardown, but its pretty
close, I think. And, for 100Mbps with reasonably recent CPUs --
anything in the last decade or so -- you shouldn't need the special
DMA tricks to get line-speed performance.)
Note that unlike rx, you can't even get the benefit of reusing
mappings... so the trade off here for bcopy vs. DMA much more
strongly favors bcopy, I think.
Again, I agree. True for modern CPU's. On slower CPU's, things are
different. The driver's objective is not to get wirespeed TCP
throughput. It's more like, "get decent throughput in few as possible
cycles". TX DMA mapping supports that objective. Every cycle not spend
in the driver is used elsewhere.
Line 2530-2553: We already have CRC32 in the kernel, you don't need
to add another one. Check out how afe_m_multicst does it in afe.
(You can use the definitions in sys/crc32.h)
I tried this but it causes nicdrv's multicast test (test04) to fail.
The reason for failure is that the CRC32 macro uses the "LSB first"
approach on the data. The card uses "MSB first" approach (in the
multicast filter). The driver has to match this approach.
Line 3021-3079: Shouldn't this be guarded by #ifdef DEBUG or
somesuch....
I removed them and set myself the goal of creating some diagnostic D
Again, generally it looks pretty good. Refactoring the DMA handling/
binding is optional, and high risk, but if you don't do that then
you *must* put guards in against the mblk's being loaned up and
outstanding at _fini time.
Most of the other comments are probably optional.
- Garrett
Joost Mulders wrote:
Good day,
I appreciate a code review for a VIA Rhine Fast Ethernet driver,
PSARC/2008/619 add a VIA Rhine Ethernet driver to Solaris
6770327 add support for VIA Rhine Fast Ethernet cards
webrev
http://cr.opensolaris.org/~joostm/onnv-vr
nicdrv journals
http://cr.opensolaris.org/~joostm/vr/nicdrv-journals
proposed manpage
http://cr.opensolaris.org/~joostm/vr/vr.7d
Thank you *very much* for your comments and time!
Joost
_______________________________________________
driver-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/driver-discuss
--
Joost Mulders + email: [email protected]
Technical Specialist + phone: +31-33-45-15701
Client Solutions + fax: +31-33-45-15734
Sun Microsystems + mobile: +31-6-5198-7268
-= Anything not done right, has to be done again =-
_______________________________________________
driver-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/driver-discuss
