Re: RFC: device attachment/detachment

[Christoph Egger <Christoph_Egger%gmx.de@localhost>]

David Young wrote:
> On Sat, May 02, 2009 at 07:53:15AM +0000, 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.
> 
> Mandating the return does not send a clear signal that leaking resources
> is not ok.  If we and we mandate the return and we commit the patch
> under consideration, breaking every architecture but amd64 in the
> process, then we will have a lot of resource leakage right off the bat,
> and many developers, cursing, will hastily add 'return 0;' to get the
> system to build again.
> 
>> 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
>> have audited". It could be imported into OpenOffice or an SQL database and
>> then one can go look for stuff to work on. Or a dumb web-app could look
>> after it.
>>
>> File,Function,Attach OK,Detach OK,Is MPSAFE,Uses device_t
>> dev/pci/iop_pci.c,iop_pci_attach,N,N,N,N
>>
>> What do you think?
> 
> I think that keeping such a CSV somewhere is a great idea!
> 
> I still think that applying Christoph's patch is a bad idea.

Should I go on in a branch ?

Christoph