Thanks for the review again!
The webrev at http://cr.opensolaris.org/~joostm/onnv-vr/ is updated.
On 27 mrt 2009, at 05:37, Garrett D'Amore wrote:
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".
Done so. I wasn't aware of proc_drvutil. Thanks.
Instead of using the private cyclic subsystem, you should use the
ddi_periodic_add() API, which does the same thing, but is public.
Again, I wasn't aware of this interface. I did the cyclic stuff too
long ago. It's ddi_periodic_add() now.
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.
Good catch! All allocations in the function have DONTWAIT now.
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().
I think xx_intr() is forced to xx_intr(void *arg1, void *arg2) by the
definition of ddi_intr_add_handler() no?, or am I unaware again?
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.
I'm (quite) sure the description for vr_mac_set_multicast() is correct
because I tested and verified it extensively. The description served
as a hypothesis for defining the function. Both the function and
description were modified during test. The datasheet is completely
silent on the subject of multicast filtering.
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
--
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
