Re: RFC: device attachment/detachment

[David Young <dyoung%pobox.com@localhost>]

On Sat, May 02, 2009 at 12:56:43AM +0200, Christoph Egger wrote:
> Attached patch modifies the driver's attachment function to
> return an error code, zero for success.
> I modified all drivers necessary to compile an amd64 GENERIC
> kernel plus bwi(4). The intended functional change is in
> subr_autoconf.c, there's no intended functional change in the
> drivers.

I agree that attach routines should return with a result code.

> W/o the patch 'drvctl -d bwi0' crashes in various places
> due to assumptions even in MI code.

From my perspective, drvctl -d bwi0 crashes because bwi_detach() is not
correct or complete.

> With the patch, bwi0 detaches:
> 
> bwi0 at pci3 dev 0 function 0: Broadcom Wireless
> bwi0: interrupting at ioapic0 pin 16
> bwi0: bwi_attach
> bwi0: bwi_power_on
> bwi0: regwin: type 0x800, rev 19, vendor 0x4243
> bwi0: BBP id 0x4321, BBP rev 0x3, BBP pkg 0
> bwi0: nregwin 5, cap 0x0864000d
> bwi0: regwin: type 0x812, rev 12, vendor 0x4243
> bwi0: MAC: rev 12
> bwi0: regwin: type 0x820, rev 4, vendor 0x4243
> bwi0: regwin: type 0x804, rev 13, vendor 0x4243
> bwi0: bus regwin already exists
> bwi0: regwin: type 0x817, rev 4, vendor 0x4243
> bwi0: bwi_power_on
> bwi0: clksrc CS_OSC
> bwi0: clkfreq min 990000, max 1010000
> bwi0: power on delay 3
> bwi0: bus rev 4
> bwi0: PCIE is enabled
> bwi0: card flags 0x3844
> bwi0: 0th led, act 91, lowact 0
> bwi0: 1th led, act 91, lowact 0
> bwi0: 2th led, act 41, lowact 0
> bwi0: 3th led, act 41, lowact 0
> bwi0: MAC was already disabled
> bwi0: PHY is linked
> bwi0: PHY type 4, rev 2, ver 5
> bwi0: unsupported PHY type 4
> bwi0: detached

But you have only nominally detached bwi0: the OS has reclaimed none of
the resources (kmem, bus space, DMA buffers, DMA maps, et cetera) that
bwi0 grabbed during attachment.

I think that this is a better way:

Add an alternate attach hook to cfattach, int (*ca_attach_ng)(device_t,
device_t, void *). Let CFATTACH_DECL3{,_NEW}() set ca_attach_ng to NULL.
Add CFATTACH_DECL4{,_NEW}() that initializes ca_attach_ng with one of
its arguments, and sets ca_attach to NULL.  Replace direct calls to
ca_attach with a wrapper that calls ca_attach_ng if it is not NULL and
returns its result code.  Otherwise, the wrapper calls ca_attach and
returns 0.

If attachment by ca_attach_ng fails, make autoconf call the driver's
detach routine to release its resources.

Gradually modernize both the attach & detach routine of each driver:
make the attach routines reliably track the resources that were
acquired.  Let the detach routines try to release only those resources
that were acquired.  Use CFATTACH_DECL4{,_NEW)(, DVF_DETACH_SHUTDOWN) to
register only those devices whose attach & detach routine DTRT.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933