Date: Sun, 8 Jun 2008 00:52:24 -0400 From: John Baldwin <jhb@freebsd.org> To: Marcel Moolenaar <marcel@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/conf NOTES files.powerpc src/sys/dev/bm if_bm.c if_bmreg.h if_bmvar.h src/sys/dev/mii lxtphy.c src/sys/modules Makefile src/sys/modules/bm Makefile src/sys/powerpc/conf GENERIC Message-ID: <200806080052.25362.jhb@freebsd.org> In-Reply-To: <200806072259.m57Mxmdo083847@repoman.freebsd.org> References: <200806072259.m57Mxmdo083847@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 07 June 2008 06:58:32 pm Marcel Moolenaar wrote:
> marcel 2008-06-07 22:58:32 UTC
>
> FreeBSD src repository
>
> Modified files:
> sys/conf NOTES files.powerpc
> sys/dev/mii lxtphy.c
> sys/modules Makefile
> sys/powerpc/conf GENERIC
> Added files:
> sys/dev/bm if_bm.c if_bmreg.h if_bmvar.h
> sys/modules/bm Makefile
> Log:
> SVN rev 179645 on 2008-06-07 22:58:32Z by marcel
>
> Add support for the Apple Big Mac (BMAC) Ethernet controller,
> found on various Apple G3 models.
>
> Submitted by: Nathan Whitehorn
A few notes from a quick perusal:
You should not need locking in the mii read/write routines as the lock is
already held at the higher levels via the ifmedia handlers and the timer
routine. Once that is done you can remove MTX_RECURSE from the mutex.
New drivers should probably just use bus_foo() (e.g. bus_read_1()) rather than
bus_space_foo(). This would mean not having the bus space tag/handle in the
softc anymore.
ether_ifattach() already sets if_data.ifi_hdrlen to the correct size for
ethernet so no need to do that in the attach routine.
In detach you should only hold the lock over the call to bm_stop(). Right now
it is subjec to deadlocks by holding it over bus_teardown_intr(), for
example. The callout_drain() must be moved after the bm_stop(). It is
also missing a call to ether_ifdetach(). The first part of detach shound
look like:
{
BM_LOCK(sc);
bm_stop(sc);
BM_UNLOCK(sc);
callout_drain(...)
ether_ifdetach(...)
bus_teardown_intr(...)
Then you can free DMA memory and I/O resources and finally destroy the mutex.
The detach routine is also missing a call to if_free() (near the bottom
usually, but anytime after the above code sequence is fine).
bm_shutdown() is missing locking.
bm_stop() should do a callout_stop() of the timer. There isn't a need to
check for link status or tx timeouts when the device is marked down.
You really don't need to have the EJUSTRETURN magic in the watchdog routine.
The callout code doesn't mind if you schedule the callout twice.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200806080052.25362.jhb>