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

[John Baldwin]

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