Re: RFC: device attachment/detachment

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

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.

Dave

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