Date: Fri, 16 Jun 2006 14:57:22 -0400 From: John Baldwin <jhb@freebsd.org> To: Ian Dowse <iedowse@iedowse.com> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c Message-ID: <200606161457.23420.jhb@freebsd.org> In-Reply-To: <200606160149.aa83404@nowhere.iedowse.com> References: <200606160149.aa83404@nowhere.iedowse.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 15 June 2006 20:49, Ian Dowse wrote: > In message <200606130856.55255.jhb@freebsd.org>, John Baldwin writes: > >On Monday 12 June 2006 20:50, Ian Dowse wrote: > >> That would bring back the original issue where a manually kldloaded > >> firmware image would be removed from the list when a driver calls > >> firmware_put(), even though the kld will remain loaded; there is > >> nothing that a driver can do to get the entry back on the list since > >> calling linker_reference_module() will not result in a call to > >> firmware_register() because the module is already (manually) loaded. > > > >No it wouldn't. :) unloadentry() is only called when we are unloading > >an explicitly loaded module from the taskqueue. That is where I think > >the 'fp->file = NULL' should be changed to 'clearentry()'. Either > >that or don't clear fp->file at all. > > Sorry for the delay in replying. True, but clearing the entry is > still assuming that there are no other linker references, and > firmware(9) doesn't really know this. For example if you manually > load a module that depends on a firmware image module, then the > firmware image module will stay loaded after unloadentry() calls > linker_file_unload(), so clearing the entry then would cause future > firmware_get() calls to fail. But unloadentry() would never unload such a module because fp->file is NULL. unloadentry() would only call clearentry() and then linker_file_unload() on an explicitly loaded firmware module. > >No. You've cleared fp->file. This means that if the other thread gets > >a reference, the firmware_unregister() will fail, but now the kernel will > >never unload this file on a subsequent firmware_put() since it won't see > >that it was explicitly loaded by the kernel since fp->file == NULL. The > >awk script patch fixes a different race where kldunload would succeed > >even though there were open references and drivers would have pointers > >into unmapped memory (or possibly mapped to something else). > > In theory whatever code was attempting to unload the module will > get back an error return and will keep its original reference on > the linker file, so that same code can succeed if it tries to unload > again later. In practice I'm sure there are bugs here ;-) For > example, firmware(9) might want to attempt to avoid clearing fp->file > if its call to linker_file_unload() fails. The problem is that in unloadentry() you are the code trying to unload the module and you ignore the error return. :) I think actually though, the best fix is to just not clear fp->file at all. If the unload succeeds, then the clearentry() in firmware_unregister() will end up clearing fp->file. If the unload fails because another thread grabbed a reference, then fp->file will remain set so that later on when that other thread does a firmware_put() we will try to unload the module in unloadentry() again. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200606161457.23420.jhb>