Joost Mulders wrote:
Hi,
I've updated the driver with comments from the 1st round.
The updated webrev is at http://cr.opensolaris.org/~joostm/onnv-vr/
Below, comments on what was changed since the last webrev.
I'd encourage you to use the new proc_drvutil mechanism in your
postinstall/preremove. I think SUNWxsvc shows an example. Likewise it
should be "preremove", not "postremove".
Instead of using the private cyclic subsystem, you should use the
ddi_periodic_add() API, which does the same thing, but is public.
Gosh, we *really* need to come up with a common MII framework that is
not ancient. :-) Your driver is another candidate one that could adapt
to a new MII framework. Its on our (my manager's) project list. :-)
I like the comment at line 1076. :-)
At 1103, why DDI_DMA_SLEEP? You have DDI_DMA_DONTWAIT in the allocation
at 1079, it seems like you could do the same here.
At line 1129... its static, and this function isn't *forced* to conform
by API. So just remove the unused argument, rather than using _NOTE().
For vr_mac_set_multicast... if you're not 100% sure of the function, it
might be simplest to configure the chip to just receive all multicast
frames. Its possibly a slight performance hit, but probably not a very
big one ... very few sites use multicast extensively.... and for 100Mbps
the complexity and possibility of bugs may not be worth it. That's the
decision I took for mxfe, btw.
We really need a big endian common CRC code as well... Probably a
potential RFE in there somewhere....
The rest of your driver looks pretty good to me. Sorry it took so long
for me to get back to you.
-- Garrett
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.
Nuke it. I'm working on a different ethernet driver for a popular
commodity chip, and it will *not* have driver.conf settings. (Every
friggin' driver always had its own unique tunables, so there was no
consistency to follow. Brussels is the way forward.)
OK. I will remove it.
Removed. There's no vr.conf anymore.
<lengthy disscussion about copy vs dma map removed>
See my earlier comments. If you want to do this, go ahead, but I
think you're mistaken. You should do some performance analysis to
see if you're really saving cpu cycles. I suspect you might be
saving as much as you think. (It will vary by packet size, as well
as specific CPU and MMU configuration, of course.)
OK. I will re-evaluate copy versus dma mapping on the VIA C3 box
measured using NFS. Is your objection with dma mappings on the TX
path, RX path or both?
I did this re-evaluation using various tools (ttcp, netperf, vdbench)
and found that there's (indeed) no gain in using dma bind on the
TX path and even no gain in using "loan up" on the RX path.
My summary conclusion (for this driver, tested on 600Mhz VIA C3):
TX: cost of ddi_dma_addr_bind_handle() is equal to cost of mcopymsg
RX: cost of the "loan up" mechanism is equal to the cost allocb+bcopy
Thus, "loan up" is removed from the RX path and dma map is removed
from the TX path. The driver now copies the data to/from pre-mapped
buffers, which significantly simplifies the driver.
Thank you, Joost
_______________________________________________
networking-discuss mailing list
[email protected]
