Cleanup and document the implementation of firmware(9) based on

a version that i posted earlier on the -current mailing list,
and subsequent feedback received.

The core of the change is just in sys/firmware.h and kern/subr_firmware.c,
while other files are just adaptation of the clients to the ABI change
(const-ification of some parameters and hiding of internal info,
so this is fully compatible at the binary level).

In detail:
- reduce the amount of information exported to clients in struct firmware,
  and constify the pointer;

- internally, document and simplify the implementation of the various
  functions, and make sure error conditions are dealt with properly.

The diffs are large, but the code is really straightforward now (i hope).

Note also that there is a subtle issue with the implementation of
firmware_register(): currently, as in the previous version, we just
store a reference to the 'imagename' argument, but we should rather
copy it because there is no guarantee that this is a static string.
I realised this while testing this code, but i prefer to fix it in
a later commit -- there is no regression with respect to the past.

Note, too, that the version in RELENG_6 has various bugs including
missing locks around the module release calls, mishandling of modules
loaded by /boot/loader, and so on, so an MFC is absolutely necessary
there.  I was just postponing it until this cleanup to avoid doing
things twice.

MFC after: 1 week

1 files changed, 272 insertions, 153 deletions

diff --git a/sys/kern/subr_firmware.c b/sys/kern/subr_firmware.c
index 29964c4..388a331 100644
--- a/sys/kern/subr_firmware.c
+++ b/sys/kern/subr_firmware.c
@@ -42,40 +42,144 @@ __FBSDID("$FreeBSD$");
 #include <sys/proc.h>
 #include <sys/module.h>
 
+/*
+ * Loadable firmware support. See sys/sys/firmware.h and firmware(9)
+ * form more details on the subsystem.
+ *
+ * 'struct firmware' is the user-visible part of the firmware table.
+ * Additional internal information is stored in a 'struct priv_fw'
+ * (currently a static array). A slot is in use if FW_INUSE is true:
+ */
+
+#define FW_INUSE(p)	((p)->file != NULL || (p)->fw.name != NULL)
+
+/*
+ * fw.name != NULL when an image is registered; file != NULL for
+ * autoloaded images whose handling has not been completed.
+ *
+ * The state of a slot evolves as follows:
+ *	firmware_register	-->  fw.name = image_name
+ *	(autoloaded image)	-->  file = module reference
+ *	firmware_unregister	-->  fw.name = NULL
+ *	(unloadentry complete)	-->  file = NULL
+ *
+ * In order for the above to work, the 'file' field must remain
+ * unchanged in firmware_unregister().
+ *
+ * Images residing in the same module are linked to each other
+ * through the 'parent' argument of firmware_register().
+ * One image (typically, one with the same name as the module to let
+ * the autoloading mechanism work) is considered the parent image for
+ * all other images in the same module. Children affect the refcount
+ * on the parent image preventing improper unloading of the image itself.
+ */
+
+struct priv_fw {
+	int		refcnt;		/* reference count */
+
+	/*
+	 * parent entry, see above. Set on firmware_register(),
+	 * cleared on firmware_unregister().
+	 */
+	struct priv_fw	*parent;
+
+	int 		flags;	/* record FIRMWARE_UNLOAD requests */
+#define FW_UNLOAD	0x100
+
+	/*
+	 * 'file' is private info managed by the autoload/unload code.
+	 * Set at the end of firmware_get(), cleared only in the
+	 * firmware_task, so the latter can depend on its value even
+	 * while the lock is not held.
+	 */
+	linker_file_t   file;	/* module file, if autoloaded */
+
+	/*
+	 * 'fw' is the externally visible image information.
+	 * We do not make it the first field in priv_fw, to avoid the
+	 * temptation of casting pointers to each other.
+	 * Use PRIV_FW(fw) to get a pointer to the cointainer of fw.
+	 * Beware, PRIV_FW does not work for a NULL pointer.
+	 */
+	struct firmware	fw;	/* externally visible information */
+};
+
+/*
+ * PRIV_FW returns the pointer to the container of struct firmware *x.
+ * Cast to intptr_t to override the 'const' attribute of x
+ */
+#define PRIV_FW(x)	((struct priv_fw *)		\
+	((intptr_t)(x) - offsetof(struct priv_fw, fw)) )
+
+/*
+ * At the moment we use a static array as backing store for the registry.
+ * Should we move to a dynamic structure, keep in mind that we cannot
+ * reallocate the array because pointers are held externally.
+ * A list may work, though.
+ */
 #define	FIRMWARE_MAX	30
-static struct firmware firmware_table[FIRMWARE_MAX];
+static struct priv_fw firmware_table[FIRMWARE_MAX];
+
+/*
+ * module release are handled in a separate task as they might sleep.
+ */
 struct task firmware_task;
+
+/*
+ * This mutex protects accesses to the firmware table.
+ */
 struct mtx firmware_mtx;
 MTX_SYSINIT(firmware, &firmware_mtx, "firmware table", MTX_DEF);
 
 /*
+ * Helper function to lookup a name.
+ * As a side effect, it sets the pointer to a free slot, if any.
+ * This way we can concentrate most of the registry scanning in
+ * this function, which makes it easier to replace the registry
+ * with some other data structure.
+ */
+static struct priv_fw *
+lookup(const char *name, struct priv_fw **empty_slot)
+{
+	struct priv_fw *fp = NULL;
+	struct priv_fw *dummy;
+	int i;
+
+	if (empty_slot == NULL)
+		empty_slot = &dummy;
+	*empty_slot = NULL;
+	for (i = 0; i < FIRMWARE_MAX; i++) {
+		fp = &firmware_table[i];
+		if (fp->fw.name != NULL && strcasecmp(name, fp->fw.name) == 0)
+			break;
+		else if (!FW_INUSE(fp))
+			*empty_slot = fp;
+	}
+	return (i < FIRMWARE_MAX ) ? fp : NULL;
+}
+
+/*
  * Register a firmware image with the specified name.  The
  * image name must not already be registered.  If this is a
  * subimage then parent refers to a previously registered
  * image that this should be associated with.
  */
-struct firmware *
+const struct firmware *
 firmware_register(const char *imagename, const void *data, size_t datasize,
-    unsigned int version, struct firmware *parent)
+    unsigned int version, const struct firmware *parent)
 {
-	struct firmware *frp = NULL;
-	int i;
+	struct priv_fw *match, *frp;
 
 	mtx_lock(&firmware_mtx);
-	for (i = 0; i < FIRMWARE_MAX; i++) {
-		struct firmware *fp = &firmware_table[i];
-
-		if (fp->name == NULL) {
-			if (frp == NULL)
-				frp = fp;
-			continue;
-		}
-		if (strcasecmp(imagename, fp->name) == 0) {
-			mtx_unlock(&firmware_mtx);
-			printf("%s: image %s already registered!\n",
-				__func__, imagename);
-			return NULL;
-		}
+	/*
+	 * Do a lookup to make sure the name is unique or find a free slot.
+	 */
+	match = lookup(imagename, &frp);
+	if (match != NULL) {
+		mtx_unlock(&firmware_mtx);
+		printf("%s: image %s already registered!\n",
+			__func__, imagename);
+		return NULL;
 	}
 	if (frp == NULL) {
 		mtx_unlock(&firmware_mtx);
@@ -83,53 +187,20 @@ firmware_register(const char *imagename, const void *data, size_t datasize,
 		    __func__, imagename);
 		return NULL;
 	}
-	frp->name = imagename;
-	frp->data = data;
-	frp->datasize = datasize;
-	frp->version = version;
-	frp->refcnt = 0;
-	frp->flags = 0;
-	if (parent != NULL)
-		parent->refcnt++;
-	frp->parent = parent;
-	frp->file = NULL;
+	bzero(frp, sizeof(frp));	/* start from a clean record */
+	frp->fw.name = imagename;
+	frp->fw.data = data;
+	frp->fw.datasize = datasize;
+	frp->fw.version = version;
+	if (parent != NULL) {
+		frp->parent = PRIV_FW(parent);
+		frp->parent->refcnt++;
+	}
 	mtx_unlock(&firmware_mtx);
-
 	if (bootverbose)
 		printf("firmware: '%s' version %u: %zu bytes loaded at %p\n",
 		    imagename, version, datasize, data);
-
-	return frp;
-}
-
-static void
-clearentry(struct firmware *fp)
-{
-	KASSERT(fp->refcnt == 0, ("image %s refcnt %u", fp->name, fp->refcnt));
-	fp->name = NULL;
-	fp->file = NULL;
-	fp->data = NULL;
-	fp->datasize = 0;
-	fp->version = 0;
-	fp->flags = 0;
-	if (fp->parent != NULL) {	/* release parent reference */
-		fp->parent->refcnt--;
-		fp->parent = NULL;
-	}
-}
-
-static struct firmware *
-lookup(const char *name)
-{
-	struct firmware *fp;
-	int i;
-
-	for (i = 0; i < FIRMWARE_MAX; i++) {
-		fp = &firmware_table[i];
-		if (fp->name != NULL && strcasecmp(name, fp->name) == 0)
-			return fp;
-	}
-	return NULL;
+	return &frp->fw;
 }
 
 /*
@@ -140,62 +211,61 @@ lookup(const char *name)
 int
 firmware_unregister(const char *imagename)
 {
-	struct firmware *fp;
-	int refcnt = 0;
+	struct priv_fw *fp;
+	int err;
 
 	mtx_lock(&firmware_mtx);
-	/*
-	 * NB: it is ok for the lookup to fail; this can happen
-	 * when a module is unloaded on last reference and the
-	 * module unload handler unregister's each of it's
-	 * firmware images.
-	 */
-	fp = lookup(imagename);
-	if (fp != NULL) {
-		refcnt = fp->refcnt;
-		if (refcnt == 0)
-			clearentry(fp);
+	fp = lookup(imagename, NULL);
+	if (fp == NULL) {
+		/*
+		 * It is ok for the lookup to fail; this can happen
+		 * when a module is unloaded on last reference and the
+		 * module unload handler unregister's each of it's
+		 * firmware images.
+		 */
+		err = 0;
+	} else if (fp->refcnt != 0) {	/* cannot unregister */
+		err = EBUSY;
+	}  else {
+		linker_file_t   x = fp->file;	/* save value */
+
+		if (fp->parent != NULL)	/* release parent reference */
+			fp->parent->refcnt--;
+		/*
+		 * Clear the whole entry with bzero to make sure we
+		 * do not forget anything. Then restore 'file' which is
+		 * non-null for autoloaded images.
+		 */
+		bzero(fp, sizeof(struct priv_fw));
+		fp->file = x;
+		err = 0;
 	}
 	mtx_unlock(&firmware_mtx);
-	return (refcnt != 0 ? EBUSY : 0);
+	return err;
 }
 
 /*
  * Lookup and potentially load the specified firmware image.
- * If the firmware is not found in the registry attempt to
- * load a kernel module with the image name.  If the firmware
- * is located a reference is returned.  The caller must release
- * this reference for the image to be eligible for removal/unload.
+ * If the firmware is not found in the registry, try to load a kernel
+ * module named as the image name.
+ * If the firmware is located, a reference is returned. The caller must
+ * release this reference for the image to be eligible for removal/unload.
  */
-struct firmware *
+const struct firmware *
 firmware_get(const char *imagename)
 {
 	struct thread *td;
-	struct firmware *fp;
+	struct priv_fw *fp;
 	linker_file_t result;
-	int requested_load = 0;
 
-again:
 	mtx_lock(&firmware_mtx);
-	fp = lookup(imagename);
-	if (fp != NULL) {
-		if (requested_load)
-			fp->file = result;
-		fp->refcnt++;
-		mtx_unlock(&firmware_mtx);
-		return fp;
-	}
+	fp = lookup(imagename, NULL);
+	if (fp != NULL)
+		goto found;
 	/*
-	 * Image not present, try to load the module holding it
-	 * or if we already tried give up.
+	 * Image not present, try to load the module holding it.
 	 */
 	mtx_unlock(&firmware_mtx);
-	if (requested_load) {
-		printf("%s: failed to load firmware image %s\n",
-		    __func__, imagename);
-		(void) linker_release_module(imagename, NULL, NULL);
-		return NULL;
-	}
 	td = curthread;
 	if (priv_check(td, PRIV_FIRMWARE_LOAD) != 0 ||
 	    securelevel_gt(td->td_ucred, 0) != 0) {
@@ -204,69 +274,105 @@ again:
 		return NULL;
 	}
 	(void) linker_reference_module(imagename, NULL, &result);
-	requested_load = 1;
-	goto again;		/* sort of an Algol-style for loop */
-}
-
-static void
-unloadentry(void *unused1, int unused2)
-{
-	struct firmware *fp;
-	linker_file_t file;
-	int i, err;
-
+	/*
+	 * After loading the module, see if the image is registered now.
+	 */
 	mtx_lock(&firmware_mtx);
-	for (;;) {
-		/* Look for an unwanted entry that we explicitly loaded. */
-		for (i = 0; i < FIRMWARE_MAX; i++) {
-			fp = &firmware_table[i];
-			if (fp->name != NULL && fp->file != NULL &&
-			    fp->refcnt == 0 &&
-			    (fp->flags & FIRMWAREFLAG_KEEPKLDREF) == 0)
-				break;
-			fp = NULL;
-		}
-		if (fp == NULL)
-			break;
-		file = fp->file;
-		/* No longer explicitly loaded. */
-		fp->file = NULL;
+	fp = lookup(imagename, NULL);
+	if (fp == NULL) {
 		mtx_unlock(&firmware_mtx);
-
-		err = linker_release_module(NULL, NULL, file);
-
-		mtx_lock(&firmware_mtx);
-		if (err) {
-			/*
-			 * If linker_release_module() 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;
-		}
+		printf("%s: failed to load firmware image %s\n",
+			__func__, imagename);
+		(void) linker_release_module(imagename, NULL, NULL);
+		return NULL;
 	}
+	fp->file = result;	/* record the module identity */
+
+found:				/* common exit point on success */
+	fp->refcnt++;
 	mtx_unlock(&firmware_mtx);
+	return &fp->fw;
 }
 
 /*
- * Release a reference to a firmware image returned by
- * firmware_get.  The reference is released and if this is
- * the last reference to the firmware image the associated
- * module may be released/unloaded.
+ * Release a reference to a firmware image returned by firmware_get.
+ * The caller may specify, with the FIRMWARE_UNLOAD flag, its desire
+ * to release the resource, but the flag is only advisory.
+ *
+ * If this is the last reference to the firmware image, and this is an
+ * autoloaded module, wake up the firmware_task to figure out what to do
+ * with the associated module.
  */
 void
-firmware_put(struct firmware *fp, int flags)
+firmware_put(const struct firmware *p, int flags)
 {
+	struct priv_fw *fp = PRIV_FW(p);
+
 	mtx_lock(&firmware_mtx);
 	fp->refcnt--;
 	if (fp->refcnt == 0) {
-		if ((flags & FIRMWARE_UNLOAD) == 0)
-			fp->flags |= FIRMWAREFLAG_KEEPKLDREF;
+		if (flags & FIRMWARE_UNLOAD)
+			fp->flags |= FW_UNLOAD;
+		if (fp->file)
+			taskqueue_enqueue(taskqueue_thread, &firmware_task);
+	}
+	mtx_unlock(&firmware_mtx);
+}
+
+/*
+ * The body of the task in charge of unloading autoloaded modules
+ * that are not needed anymore.
+ * Images can be cross-linked so we may need to make multiple passes,
+ * but the time we spend in the loop is bounded because we clear entries
+ * as we touch them.
+ */
+static void
+unloadentry(void *unused1, int unused2)
+{
+	int limit = FIRMWARE_MAX;
+	int i;	/* current cycle */
+
+	mtx_lock(&firmware_mtx);
+	/*
+	 * Scan the table. limit is set to make sure we make another
+	 * full sweep after matching an entry that requires unloading.
+	 */
+	for (i = 0; i < limit; i++) {
+		struct priv_fw *fp;
+		int err;
+
+		fp = &firmware_table[i % FIRMWARE_MAX];
+		if (fp->fw.name == NULL || fp->file == NULL ||
+		    fp->refcnt != 0 || (fp->flags & FW_UNLOAD) == 0)
+			continue;
+
+		/*
+		 * Found an entry. Now:
+		 * 1. bump up limit to make sure we make another full round;
+		 * 2. clear FW_UNLOAD so we don't try this entry again.
+		 * 3. release the lock while trying to unload the module.
+		 * 'file' remains set so that the entry cannot be reused
+		 * in the meantime (it also means that fp->file will
+		 * not change while we release the lock).
+		 */
+		limit = i + FIRMWARE_MAX;	/* make another full round */
+		fp->flags &= ~FW_UNLOAD;	/* do not try again */
+
+		mtx_unlock(&firmware_mtx);
+		err = linker_release_module(NULL, NULL, fp->file);
+		mtx_lock(&firmware_mtx);
+
+		/*
+		 * We rely on the module to call firmware_unregister()
+		 * on unload to actually release the entry.
+		 * If err = 0 we can drop our reference as the system
+		 * accepted it. Otherwise unloading failed (e.g. the
+		 * module itself gave an error) so our reference is
+		 * still valid.
+		 */
+		if (err == 0)
+			fp->file = NULL; 
 	}
-	if (fp->file)
-		taskqueue_enqueue(taskqueue_thread, &firmware_task);
 	mtx_unlock(&firmware_mtx);
 }
 
@@ -276,21 +382,34 @@ firmware_put(struct firmware *fp, int flags)
 static int
 firmware_modevent(module_t mod, int type, void *unused)
 {
-	struct firmware *fp;
-	int i;
+	struct priv_fw *fp;
+	int i, err = EINVAL;
 
 	switch (type) {
 	case MOD_LOAD:
 		TASK_INIT(&firmware_task, 0, unloadentry, NULL);
 		return 0;
+
 	case MOD_UNLOAD:
+		/* request all autoloaded modules to be released */
+		mtx_lock(&firmware_mtx);
 		for (i = 0; i < FIRMWARE_MAX; i++) {
 			fp = &firmware_table[i];
-			fp->flags &= ~FIRMWAREFLAG_KEEPKLDREF;
+			fp->flags |= FW_UNLOAD;;
 		}
+		mtx_unlock(&firmware_mtx);
 		taskqueue_enqueue(taskqueue_thread, &firmware_task);
 		taskqueue_drain(taskqueue_thread, &firmware_task);
-		return 0;
+		for (i = 0; i < FIRMWARE_MAX; i++) {
+			fp = &firmware_table[i];
+			if (fp->fw.name != NULL) {
+				printf("%s: image %p ref %d still active slot %d\n",
+					__func__, fp->fw.name,
+					fp->refcnt,  i);
+				err = EINVAL;
+			}
+		}
+		return err;
 	}
 	return EINVAL;
 }