From 5d692ded600aa0bf0fcd5a12e2278b4573879b68 Mon Sep 17 00:00:00 2001 From: Jan Sucan Date: Thu, 25 May 2017 12:13:59 +0000 Subject: [PATCH] firmware: Remove embedding of multiple images in one module. This patch removes possibility of embedding more than one firmware image in one kernel module through the parent reference in the firmware_register() function. This patch is a preparation for firmware subsystem modification for moving firmware images from kernel modules to userland. The mechanism is not used and it can be functionally fully replaced by putting each firmware image in its own module. Removing the functionality significantly simplifies handling of firmware images. If firmware images are moved to userland the logical grouping of modules could be expressed by putting the related firmware images into one directory if needed. --- share/examples/kld/firmware/wrap-fw_module.sh | 2 +- share/man/man9/firmware.9 | 13 +---- sys/dev/disk/ispfw/ispfw.c | 2 +- sys/kern/subr_firmware.c | 49 ++++------------- sys/sys/firmware.h | 20 +++---- sys/tools/fw_stub.awk | 79 ++++++++++----------------- 6 files changed, 54 insertions(+), 111 deletions(-) diff --git a/share/examples/kld/firmware/wrap-fw_module.sh b/share/examples/kld/firmware/wrap-fw_module.sh index 62436cc32..9021c24e9 100755 --- a/share/examples/kld/firmware/wrap-fw_module.sh +++ b/share/examples/kld/firmware/wrap-fw_module.sh @@ -70,7 +70,7 @@ ${MODNAME}_fw_modevent(module_t mod, int type, void *unused) int error; switch (type) { case MOD_LOAD: - fp = firmware_register("${MODNAME}", _binary_${FWSYM}_start , (size_t)(_binary_${FWSYM}_end - _binary_${FWSYM}_start), 0, NULL); + fp = firmware_register("${MODNAME}", _binary_${FWSYM}_start , (size_t)(_binary_${FWSYM}_end - _binary_${FWSYM}_start), 0); if (fp == NULL) goto fail_0; return (0); diff --git a/share/man/man9/firmware.9 b/share/man/man9/firmware.9 index df87d098f..8fece922a 100644 --- a/share/man/man9/firmware.9 +++ b/share/man/man9/firmware.9 @@ -51,7 +51,6 @@ struct firmware { .Fa "const void *data" .Fa "size_t datasize" .Fa "unsigned int version" -.Fa "const struct firmware *parent" .Fc .Ft int .Fn firmware_unregister "const char *imagename" @@ -195,12 +194,6 @@ by the sysctl variable .Nm kern.module_path which on most systems defaults to .Nm /boot/kernel;/boot/modules . -.Pp -Note that in case a module contains multiple images, -the caller should first request a -.Fn firmware_get -for the first image contained in the module, followed by requests -for the other images. .Sh BUILDING FIRMWARE LOADABLE MODULES A firmware module is built by embedding the .Nm firmware image @@ -219,9 +212,9 @@ by simply writing a Makefile with the following entries: .include .Ed -where KMOD is the basename of the module; FIRMWS is a list of -colon-separated tuples indicating the image_file's to be embedded -in the module, the imagename and version of each firmware image. +where KMOD is the basename of the module; FIRMWS is a colon-separated +tuple indicating the image_file to be embedded in the module, the +imagename and version of the firmware image. .Pp If you need to embed firmware images into a system, you should write appropriate entries in the diff --git a/sys/dev/disk/ispfw/ispfw.c b/sys/dev/disk/ispfw/ispfw.c index c61e3af49..3c1ac0537 100644 --- a/sys/dev/disk/ispfw/ispfw.c +++ b/sys/dev/disk/ispfw/ispfw.c @@ -143,7 +143,7 @@ static int isp_2500_multi_loaded; break; \ if (firmware_register(#token, token##_risc_code, \ token##_risc_code[3] * sizeof(token##_risc_code[3]), \ - ISPFW_VERSION, NULL) == NULL) { \ + ISPFW_VERSION) == NULL) { \ kprintf("%s: unable to register firmware <%s>\n", \ MODULE_NAME, #token); \ break; \ diff --git a/sys/kern/subr_firmware.c b/sys/kern/subr_firmware.c index 25939d70a..6ea126e8f 100644 --- a/sys/kern/subr_firmware.c +++ b/sys/kern/subr_firmware.c @@ -69,24 +69,11 @@ * * 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 @@ -165,13 +152,11 @@ lookup(const char *name, struct priv_fw **empty_slot) /* * 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. + * image name must not already be registered. */ const struct firmware * firmware_register(const char *imagename, const void *data, size_t datasize, - unsigned int version, const struct firmware *parent) + unsigned int version) { struct priv_fw *match, *frp; @@ -197,10 +182,7 @@ firmware_register(const char *imagename, const void *data, size_t datasize, frp->fw.data = data; frp->fw.datasize = datasize; frp->fw.version = version; - if (parent != NULL) { - frp->parent = PRIV_FW(parent); - frp->parent->refcnt++; - } + lockmgr(&firmware_lock, LK_RELEASE); if (bootverbose) kprintf("firmware: '%s' version %u: %zu bytes loaded at %p\n", @@ -225,8 +207,8 @@ firmware_unregister(const char *imagename) /* * 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. + * module unload handler unregisters it's firmware + * image. */ err = 0; } else if (fp->refcnt != 0) { /* cannot unregister */ @@ -234,8 +216,6 @@ firmware_unregister(const char *imagename) } 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 @@ -429,33 +409,28 @@ EVENTHANDLER_DECLARE(mountroot, firmware_mountroot); static void unloadentry(void *unused1, int unused2) { - int limit = FIRMWARE_MAX; - int i; /* current cycle */ + int i; lockmgr(&firmware_lock, LK_EXCLUSIVE); - /* - * 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++) { + /* Scan the table. */ + for (i = 0; i < FIRMWARE_MAX; i++) { struct priv_fw *fp; int err; - fp = &firmware_table[i % FIRMWARE_MAX]; + fp = &firmware_table[i]; 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. + * 1. clear FW_UNLOAD so we don't try this entry again + * if unloading will be unsuccessful. + * 2. 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 */ lockmgr(&firmware_lock, LK_RELEASE); diff --git a/sys/sys/firmware.h b/sys/sys/firmware.h index 31b7886be..558e12495 100644 --- a/sys/sys/firmware.h +++ b/sys/sys/firmware.h @@ -33,18 +33,14 @@ * The firmware abstraction provides an interface for loading firmware * images into the kernel and making them available to clients. * - * Firmware images are usually embedded in kernel loadable modules that can - * be loaded on-demand or pre-loaded as desired. Modules may contain - * one or more firmware images that are stored as opaque data arrays - * and registered with a unique string name. Clients request - * firmware by name, and are returned a struct firmware * below on success. - * The kernel keeps track of references to firmware images to allow/prevent - * module/data unload. + * Firmware images are usually embedded in kernel loadable modules + * that can be loaded on-demand or pre-loaded as desired. Modules may + * contain one firmware image that is stored as opaque data array and + * registered with a unique string name. Clients request firmware by + * name, and are returned a struct firmware * below on success. The + * kernel keeps track of references to firmware images to + * allow/prevent module/data unload. * - * When multiple images are stored in one module, the first image is - * treated as the master with the other images holding references - * to it. This means that to unload the module each dependent/subimage - * must first have its references removed. * In order for automatic loading to work, the master image must have * the same name as the module it is embedded into. */ @@ -56,7 +52,7 @@ struct firmware { }; const struct firmware *firmware_register(const char *, - const void *, size_t, unsigned int, const struct firmware *); + const void *, size_t, unsigned int); int firmware_unregister(const char *); const struct firmware *firmware_get(const char *); #define FIRMWARE_UNLOAD 0x0001 /* unload if unreferenced */ diff --git a/sys/tools/fw_stub.awk b/sys/tools/fw_stub.awk index 79111ce79..4f1a94559 100644 --- a/sys/tools/fw_stub.awk +++ b/sys/tools/fw_stub.awk @@ -33,7 +33,7 @@ function usage () { - print "usage: fw_stub * [-l name] [-m modname] [-c outfile]"; + print "usage: fw_stub [-l name] [-m modname] [-c outfile]"; exit 1; } @@ -98,18 +98,22 @@ for (i = 1; i < ARGC; i++) { } else usage(); } - } else { + } else if (num_files == 0) { split(ARGV[i], curr, ":"); - filenames[num_files] = curr[1]; + filename = curr[1]; if (length(curr[2]) > 0) - shortnames[num_files] = curr[2]; + shortname = curr[2]; else - shortnames[num_files] = curr[1]; + shortname = curr[1]; if (length(curr[3]) > 0) - versions[num_files] = int(curr[3]); + version = int(curr[3]); else - versions[num_files] = 0; + version = 0; num_files++; + } else { + # The argument is a firmware image specification + # but we already have one + usage(); } } @@ -133,21 +137,16 @@ if (opt_l) { printc("static long " opt_l "_license_ack = 0;"); } -for (file_i = 0; file_i < num_files; file_i++) { - symb = filenames[file_i]; - # '-', '.' and '/' are converted to '_' by ld/objcopy - gsub(/-|\.|\//, "_", symb); - printc("extern char _binary_" symb "_start[], _binary_" symb "_end[];"); -} +symb = filename; +# '-', '.' and '/' are converted to '_' by ld/objcopy +gsub(/-|\.|\//, "_", symb); +printc("extern char _binary_" symb "_start[], _binary_" symb "_end[];"); printc("\nstatic int\n"\ modname "_fw_modevent(module_t mod, int type, void *unused)\ {\ const struct firmware *fp;"); -if (num_files > 1) - printc("\tconst struct firmware *parent;"); - printc("\tint error;\ switch (type) {\ case MOD_LOAD:\n"); @@ -162,50 +161,30 @@ if (opt_l) { }\n"); } -for (file_i = 0; file_i < num_files; file_i++) { - short = shortnames[file_i]; - symb = filenames[file_i]; - version = versions[file_i]; - # '-', '.' and '/' are converted to '_' by ld/objcopy - gsub(/-|\.|\//, "_", symb); +short = shortname; +symb = filename; +version = version; +# '-', '.' and '/' are converted to '_' by ld/objcopy +gsub(/-|\.|\//, "_", symb); - reg = "\t\tfp = "; - reg = reg "firmware_register(\"" short "\", _binary_" symb "_start , "; - reg = reg "(size_t)(_binary_" symb "_end - _binary_" symb "_start), "; - reg = reg version ", "; +reg = "\t\tfp = "; +reg = reg "firmware_register(\"" short "\", _binary_" symb "_start , "; +reg = reg "(size_t)(_binary_" symb "_end - _binary_" symb "_start), "; +reg = reg version ");"; - if (file_i == 0) - reg = reg "NULL);"; - else - reg = reg "parent);"; +printc(reg); - printc(reg); - - printc("\t\tif (fp == NULL)"); - printc("\t\t\tgoto fail_" file_i ";"); - if (file_i == 0 && num_files > 1) - printc("\t\tparent = fp;"); -} +printc("\t\tif (fp == NULL)"); +printc("\t\t\tgoto fail;"); printc("\t\treturn (0);"); -for (file_i = num_files - 1; file_i > 0; file_i--) { - printc("fail_" file_i ":") - printc("\t\t(void)firmware_unregister(\"" shortnames[file_i - 1] "\");"); -} - -printc("\tfail_0:"); +printc("\tfail:"); printc("\t\treturn (ENXIO);"); printc("\tcase MOD_UNLOAD:"); -for (file_i = 1; file_i < num_files; file_i++) { - printc("\t\terror = firmware_unregister(\"" shortnames[file_i] "\");"); - printc("\t\tif (error)"); - printc("\t\t\treturn (error);"); -} - -printc("\t\terror = firmware_unregister(\"" shortnames[0] "\");"); +printc("\t\terror = firmware_unregister(\"" shortname "\");"); printc("\t\treturn (error);\ }\ -- 2.12.1