Date: Mon, 19 Jun 2006 19:24:00 +0100 From: Ian Dowse <iedowse@iedowse.com> To: John Baldwin <jhb@freebsd.org> 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: <200606191924.aa78290@nowhere.iedowse.com> In-Reply-To: Your message of "Mon, 19 Jun 2006 09:15:50 EDT." <200606190915.50962.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <200606190915.50962.jhb@freebsd.org>, John Baldwin writes:
>On Friday 16 June 2006 20:47, Ian Dowse wrote:
>> - driver calls firmware_get, firmware image loaded and fp->file set to non-NULL
>> - manually kldload some_module_that_depends_on_firmware_image
>> - driver calls firmware_put, unloadentry called and sets fp->file = NULL
>
>In practice no modules depend on firmware modules. :) I think we should
>take the approach of not clearing fp->file in unloadentry() however.
>That would result in correct behavior in every case I can think of (or as
>close to correct as you can get). In the above case the
>linker_file_unload() would have fail leaving the firmware module around.
How about the following patch? In the above case linker_file_unload()
would actually succeed because the linker file reference count is
just dropping from 2 to 1, so we'd get a negative reference count
later if we kept fp->file non-NULL after it returns.
Ian
Index: subr_firmware.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/subr_firmware.c,v
retrieving revision 1.2
diff -u -r1.2 subr_firmware.c
--- subr_firmware.c 10 Jun 2006 17:04:07 -0000 1.2
+++ subr_firmware.c 19 Jun 2006 18:15:21 -0000
@@ -206,7 +206,7 @@
{
struct firmware *fp;
linker_file_t file;
- int i;
+ int i, err;
mtx_lock(&firmware_mtx);
for (;;) {
@@ -226,9 +226,18 @@
fp->file = NULL;
mtx_unlock(&firmware_mtx);
- linker_file_unload(file, LINKER_UNLOAD_NORMAL);
-
+ err = linker_file_unload(file, LINKER_UNLOAD_NORMAL);
mtx_lock(&firmware_mtx);
+ if (err) {
+ /*
+ * If linker_file_unload() failed then we still hold
+ * a reference on the module so it should not be
+ * possible for it to go away or be re-registered.
+ */
+ KASSERT(fp->file == NULL,
+ ("firmware entry reused while referenced!"));
+ fp->file = file;
+ }
}
mtx_unlock(&firmware_mtx);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200606191924.aa78290>