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.)
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.)
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.
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().)
Line 640: Historically bcopy is preferred over memcpy in the kernel.
(Minor stylistic nit.) (IIRC, memcpy wasn't available in the kernel
until S10.)
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.
Line 944: minor language nit..."out of this list" not "this lists".
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.)
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.)
Line 1287: I see you're using desballoc. For a 100M driver, I think
this adds complexity that is questionable. 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 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.)
Line 1680: This conflicts with the information in the man page
indicating the card doesn't transmit flow control frames.
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.
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)
Line 3021-3079: Shouldn't this be guarded by #ifdef DEBUG or somesuch....
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
>
_______________________________________________
networking-discuss mailing list
[email protected]
