headers
Christoph Egger | 2 May 2009 00:56
Picon
Picon

RFC: device attachment/detachment

Hi,

Drivers attaches in the device tree independent if their attachment
function failed or not. The autoconf framework isn't aware of this,
because the attachment function doesn't return an error code.

dyoung <at>  started to work on the drivers to detach the drivers
on shutdown/reboot. The drivers' detach function doesn't know if
the attachment failed or not. It only can carefully
check which resource was allocated and which not and free the
allocated ones.

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.

The full blown patch (500KB) is at
http://www.netbsd.org/~cegger/autoconf.diff

bwi(4) attachment always fails on my machine
due to lack of Draft-N support in a very early support.

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

With the patch, bwi0 detaches:
(Continue reading)

Permalink | Reply | 14 comments |
headers
Quentin Garnier | 2 May 2009 01:18

Re: RFC: device attachment/detachment

On Sat, May 02, 2009 at 12:56:43AM +0200, Christoph Egger wrote:
> Drivers attaches in the device tree independent if their attachment
> function failed or not. The autoconf framework isn't aware of this,
> because the attachment function doesn't return an error code.
> 
> dyoung <at>  started to work on the drivers to detach the drivers
> on shutdown/reboot. The drivers' detach function doesn't know if
> the attachment failed or not. It only can carefully
> check which resource was allocated and which not and free the
> allocated ones.

No matter what happens, one has to be careful not to free resources it
didn't alloc.  I don't see how anything could change that.

> Attached patch modifies the driver's attachment function to
> return an error code, zero for success.

While I understand where this comes from, autoconf(9) has more short-
comings than just that one, and fixing them will require touching every
single driver in the tree, too.  I'd rather see that done just once.

[...]
>  <at>  <at>  -1377,6 +1381,15  <at>  <at>  config_attach_loc(device_t parent, cfdat
>  			}
>  		}
>  	}
> +
> +#if defined(SPLASHSCREEN) && defined(SPLASHSCREEN_PROGRESS)
> +	if (splash_progress_state)
> +		splash_progress_update(splash_progress_state);
(Continue reading)

Permalink | Reply | 6 comments |
headers
David Young | 2 May 2009 02:21
Picon
Favicon

Re: RFC: device attachment/detachment

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
(Continue reading)

Permalink | Reply | 6 comments |
headers
Andrew Doran | 2 May 2009 09:53
Picon

Re: RFC: device attachment/detachment

On Fri, May 01, 2009 at 07:21:18PM -0500, David Young wrote:

> 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.

Yes, thanks for looking at this!

> 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.

I dislike this somewhat this because it's open-ended and does not improve
the existing standard ("panicing or leaking resources is OK"). That is,
anyone can go cut-n-paste and get the old mechanism. By mandating the return
we would be giving a stronger signal about what's required.

Here is a suggestion of another way:

Apply Christoph's patch, and generate a list of all the attach functions in
tree along with corresponding file name. Put the list into a csv somewhere
(src/sys/doc? unimportant really) and say "these are the drivers that we
(Continue reading)

Permalink | Reply | 4 comments |
headers
Christoph Egger | 2 May 2009 09:55
Picon
Picon

Re: RFC: device attachment/detachment

Quentin Garnier wrote:
> On Sat, May 02, 2009 at 12:56:43AM +0200, Christoph Egger wrote:
>> Drivers attaches in the device tree independent if their attachment
>> function failed or not. The autoconf framework isn't aware of this,
>> because the attachment function doesn't return an error code.
>>
>> dyoung <at>  started to work on the drivers to detach the drivers
>> on shutdown/reboot. The drivers' detach function doesn't know if
>> the attachment failed or not. It only can carefully
>> check which resource was allocated and which not and free the
>> allocated ones.
> 
> No matter what happens, one has to be careful not to free resources it
> didn't alloc.  I don't see how anything could change that.
> 
>> Attached patch modifies the driver's attachment function to
>> return an error code, zero for success.
> 
> While I understand where this comes from, autoconf(9) has more short-
> comings than just that one, and fixing them will require touching every
> single driver in the tree, too.  I'd rather see that done just once.

Can you enlist all shortcomings, please ?

> [...]
>>  <at>  <at>  -1377,6 +1381,15  <at>  <at>  config_attach_loc(device_t parent, cfdat
>>  			}
>>  		}
>>  	}
>> +
(Continue reading)

Permalink | Reply | 5 comments |
headers
Christoph Egger | 2 May 2009 09:59
Picon
Picon

Re: RFC: device attachment/detachment

David Young wrote:
> 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
(Continue reading)

Permalink | Reply | 0 comments |
headers
Christoph Egger | 2 May 2009 10:10
Picon
Picon

Re: RFC: device attachment/detachment

Andrew Doran wrote:
> On Fri, May 01, 2009 at 07:21:18PM -0500, David Young wrote:
> 
>> 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.
> 
> Yes, thanks for looking at this!
> 
>> 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.
> 
> I dislike this somewhat this because it's open-ended and does not improve
> the existing standard ("panicing or leaking resources is OK"). That is,
> anyone can go cut-n-paste and get the old mechanism. By mandating the return
> we would be giving a stronger signal about what's required.

I agree on that. My patch changes the semantic in one go.
Note, that my patch breaks the build of all
kernels but amd64 GENERIC. I can't change all drivers alone and need
help for this. Given the fact of build breakage, I am sure I get help
from a lot of people, *if* I can apply my patch as Andrew suggests
(Continue reading)

Permalink | Reply | 0 comments |
headers
Quentin Garnier | 2 May 2009 13:41

Re: RFC: device attachment/detachment

On Sat, May 02, 2009 at 09:55:01AM +0200, Christoph Egger wrote:
> Quentin Garnier wrote:
> > On Sat, May 02, 2009 at 12:56:43AM +0200, Christoph Egger wrote:
> >> Drivers attaches in the device tree independent if their attachment
> >> function failed or not. The autoconf framework isn't aware of this,
> >> because the attachment function doesn't return an error code.
> >>
> >> dyoung <at>  started to work on the drivers to detach the drivers
> >> on shutdown/reboot. The drivers' detach function doesn't know if
> >> the attachment failed or not. It only can carefully
> >> check which resource was allocated and which not and free the
> >> allocated ones.
> > 
> > No matter what happens, one has to be careful not to free resources it
> > didn't alloc.  I don't see how anything could change that.
> > 
> >> Attached patch modifies the driver's attachment function to
> >> return an error code, zero for success.
> > 
> > While I understand where this comes from, autoconf(9) has more short-
> > comings than just that one, and fixing them will require touching every
> > single driver in the tree, too.  I'd rather see that done just once.
> 
> Can you enlist all shortcomings, please ?

In a nutshell, autoconf(9) doesn't allow a device to exist without a
driver attached to it.  That's why there is an on-going effort to split
the struct device from a driver's softc.  It is a necessary step to more
radical changes to the autoconf(9) API.
(Continue reading)

Permalink | Reply | 3 comments |
headers
Christoph Egger | 1 May 2009 19:34
Picon
Picon

RFC: device attachment/detachment


Hi,

Drivers attaches in the device tree independent if their attachment
function failed or not. The autoconf framework isn't aware of this,
because the attachment function doesn't return an error code.

dyoung <at>  started to work on the drivers to detach the drivers
on shutdown/reboot. The drivers' detach function doesn't know if
the attachment failed or not. It only can carefully
check which resource was allocated and which not and free the
allocated ones.

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.

bwi(4) attachment always fails on my machine
due to lack of Draft-N support in a very early support.

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

With attached patch, bwi0 detaches:

bwi0 at pci3 dev 0 function 0: Broadcom Wireless
bwi0: interrupting at ioapic0 pin 16
(Continue reading)

Permalink | Reply | 0 comments |
headers
Izumi Tsutsui | 2 May 2009 14:43
Picon
Gravatar

Re: RFC: device attachment/detachment

> On x86, device_register() is used to set bootdev. When prompted for
> the boot device, the failed devices shouldn't be listed.

Check evbarm, iyonix, prep, and sgimips etc.

---
Izumi Tsutsui
Permalink | Reply | 0 comments |

Gmane