Re: CFT: vr(4)

On Wed, Feb 13, 2008 at 01:13:53AM -0500, Andrew Atrens wrote:
 > -----BEGIN PGP SIGNED MESSAGE-----
 > Hash: SHA1
 > 
 > 
 > Pyun,
 > 
 > I've ported your driver to DragonFlyBSD and am running it on a PC Engines ALIX1.C
 > board (www.pcengines.ch) which has this nic -
 > 
 > 
 > vr0: <VIA VT6105M Rhine III 10/100BaseTX> port 0xfc00-0xfcff mem 0xeffff000-0xeffff0ff irq 10 at device 13.0 on pci0
 > vr0: Quirks: 0x6
 > vr0: Revision: 0x96
 > vr0: MAC address: 00:0d:b9:0c:d6:00
 > 
 > vr0_at_pci0:13:0:  class=0x020000 card=0x01061106 chip=0x30531106 rev=0x96 hdr=0x00
 >     vendor   = 'VIA Technologies Inc'
 >     device   = 'VT6105M Rhine III Management Adapter'
 >     class    = network
 >     subclass = ethernet
 > 
 > Your driver is slightly faster than the old vr driver. I'm using netstrain/netstraind to measure
 > throughput. I also find that interrupt load is a little bit higher than the old driver, but maybe
 > that is because throughput is also higher.

Probably yes.

 > 
 > One of the problems that I ran into with this new driver was Tx/Rx buffer alignment. It seems that
 > my chip needs longword alignment for both transmit and receive buffers.. So I added the VR_Q_NEEDALIGN

VIA Rhine II or III have no Tx buffer alignment restriction as it
does on Rhine I(The datasheet is wrong on this information. You may
also noticed there lots of wrong information the datasheet.) So
Rhine III should work without any Tx buffer alignment restrictions.
But all Rhine family requires Rx buffer alignment on 4 bytes
boundary. Maybe alignment restriction of bus_dma_tag_create(9)
is not honored in DragonFlyBSD?

 > to the quirks table and then saw -
 > 
 > vr0: Quirks: 0x7
 > 
 > But that didn't fix the problem.  I noticed that in vr_newbuf you have the following code -
 > 
 >         m->m_len = m->m_pkthdr.len = MCLBYTES;
 >         m_adj(m, sizeof(uint64_t));
 > 
 >         error = bus_dmamap_load_mbuf_sg(..
 > 
 > I'm not sure of the purpose of the call to m_adj here, but is not providing longword alignment -
 > it is just adding some padding.
 > 

It's for alignment fixup code for strict-alignment architectures
such as sparc64 as vr(4) should copy entire frame to align its
payload(Overhauled vr(4) now works on sparc64).

 > Similarly in vr_encap(), you have the following code -
 > 
 >        if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0) {
 >                 m = m_defrag(*m_head, MB_DONTWAIT);
 >                 if (m == NULL) {
 >                         m_freem(*m_head);
 >                         *m_head = NULL;
 >                         return (ENOBUFS);
 >                 }
 >                 *m_head = m;
 >         }
 > 
 > In DragonFlyBSD at least doing an m_defrag does not guarantee longword alignment - and it's slow.
 > 

Sorry, I don't know DragonFlyBSD's internals. m_defarg(9) always
allocates a new mbuf so it's guaranteed the m_data is aligned
on 4 bytes boundary in FreeBSD.

 > So, for DragonFly, I added this routine - kind of unelegant, but functional -
 > 
 > 
 > /* try and fixup longword alignment */
 > static __inline void
 > vr_fixup_lw(struct mbuf *m)
 > {
 >         uint8_t         *src, *dst;
 >         int                     i;
 >         src = mtod(m, uint8_t *);
 > 
 >         if ( (dst = (uint8_t *)((int)src & ~(sizeof(uint64_t)-1))) == 0 )
 >                 return;
 > 
 >         if ( M_LEADINGSPACE(m) < (dst - src) ) {
 >                 kprintf("vr_fixup_lw: cannot repair!\n");
 >                 return;
 >         }
 > 
 >         m->m_data = dst;
 > 
 >         for (i = 0; i < m->m_len; i++)
 >                 *dst++ = *src++;
 > }
 > 
 > 
 > Now, in vr_encap() I instead have the following -
 > 
 >        if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0)
 > 		vr_fixed_lw(m);
 > 
 > 
 > and in vr_newbuf() -
 > 
 >         m->m_len = m->m_pkthdr.len = MCLBYTES;
 >         m_adj(m, sizeof(uint64_t));
 > 
 > 	if ((sc->vr_quirks & VR_Q_NEEDALIGN) != 0)
 > 		vr_fixup_lw(m);
 > 
 >         error = bus_dmamap_load_mbuf_sg(..
 > 
 > 
 > These two changes nicely fix my alignment issues. :o)
 > 
 > 
 > Finally I had a problem with my phy device .. it appeared to be wedged, I
 > could receive but not send.
 > 
 > The problem seemed to be in vr_init_locked(), towards the bottom. You had -
 > 
 > 	sc->vr_link = 0;
 > 	mii_mediachg(mii)
 > 
 > 	ifp->if...
 > 
 > but this didn't work. I discovered that running -
 > 
 > # ifconfig vr0 media auto
 > 
 > will correct the problem, and so I added the phy_resets() found in vr_ifmedia_upd()
 > into vr_init_locked(), so the new code looks like -
 > 
 > 	sc->vr_link = 0;
 >         if (mii->mii_instance) {
 >                 struct mii_softc *miisc;
 >                 LIST_FOREACH(miisc, &mii->mii_phys, mii_list)
 >                         mii_phy_reset(miisc);
 >         }
 >         mii_mediachg(mii);
 > 
 > 

Does it use ukphy(4)? Overhauld vr(4) relys on correct link state
change report from mii layer. Does DragonFlyBSD also have the
feature?

 > Normally I would provide a diff against FreeBSD sources, but I have no place to easily
 > test this on FreeBSD, and these 4 changes (if you count the change to the quirks table)
 > are quite small.
 > 
 > 
 > Nice job on the driver!
 > 
 > - --Andrew.
 > 
 > 
 > Pyun YongHyeon wrote:
 > > Dear all,
 > > 
 > > Here is overhauled vr(4) that shall address all known issues. PR
 > > database showed vr(4) is not stable enough under high load and
 > > link state handling didn't work as expected as well as its poor
 > > performance. I've tried hard to fix the bugs reported in PR
 > > database for several months. It tooked more time than I had
 > > planned when I received donated hardware.
 > > vr(4) will now
 > >  - work on all architectures.
 > >  - have better Rx performance.
 > >  - have more robust error recovery.
 > >  - reliably detect link state changes.
 > >  - support 32bit perfect multicast filtering for VT6105M.
 > >  - support WOL.
 > > 
 > > It seems that the overhauled vr(4) runs well under my limited test
 > > environments. Several users already reported success. Because there
 > > are three kinds of Rhine family and lots of variants for the
 > > hardware, I'd like to hear feedback from users priror to commit.
 > > You can get the latest vr(4) at the following URL.
 > > 
 > > http://people.freebsd.org/~yongari/vr/if_vr.c
 > > http://people.freebsd.org/~yongari/vr/if_vrreg.h
 > > 
 > > I wouldn't be available for 7 ~ 10 days from Feb 5, so please don't
 > > expect quick reply.
 > > 
 > 
 > -----BEGIN PGP SIGNATURE-----
 > Version: GnuPG v2.0.4 (FreeBSD)
 > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
 > 
 > iD8DBQFHsokP8It2CaCdeMwRAjGTAJ9Plc/+i3GWor1uGgsnTihKWEezcgCeOyTP
 > alM6RjNsSjxUk/4DKdIQuds=
 > =hdHZ
 > -----END PGP SIGNATURE-----

-- 
Regards,
Pyun YongHyeon