Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status | From | Daniel Wagner <> | Date | Mon, 29 Aug 2016 11:50:45 +0200 |
| |
On 08/25/2016 07:50 PM, Luis R. Rodriguez wrote: >> +#else /* CONFIG_FW_LOADER_USER_HELPER */ >> + >> +static int loading_timeout = 60; >> +#define firmware_loading_timeout() (loading_timeout * HZ) >> + >> +#define fw_status_wait_timeout(fw_st, long) 0 > > The timeout makes 0 sense for when !CONFIG_FW_LOADER_USER_HELPER so can > we do away with adding a silly 60 value to an int here and > the silly value of (loading_timeout * HZ) ? Its not used so its not > clear to me why this is here.
So the main reason that silly timeout is needed is the usage of it in device_cache_fw_images(). I suggest we add a timeout argument to _request_firmware() and use the right timeout value at that level.
That allows to move the loading_timeout into the CONFIG_FW_LOADER_USER_HELPER section.
What do you think?
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 22d1760..afb6d93 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -1132,10 +1132,9 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device, static int _request_firmware(const struct firmware **firmware_p, const char *name, struct device *device, void *buf, size_t size, - unsigned int opt_flags) + unsigned int opt_flags, long timeout) { struct firmware *fw = NULL; - long timeout; int ret; if (!firmware_p) @@ -1151,7 +1150,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, goto out; ret = 0; - timeout = firmware_loading_timeout(); if (opt_flags & FW_OPT_NOWAIT) { timeout = usermodehelper_read_lock_wait(timeout); if (!timeout) { @@ -1226,7 +1224,8 @@ request_firmware(const struct firmware **firmware_p, const char *name, /* Need to pin this module until return */ __module_get(THIS_MODULE); ret = _request_firmware(firmware_p, name, device, NULL, 0, - FW_OPT_UEVENT | FW_OPT_FALLBACK); + FW_OPT_UEVENT | FW_OPT_FALLBACK, + firmware_loading_timeout()); module_put(THIS_MODULE); return ret; } @@ -1250,7 +1249,8 @@ int request_firmware_direct(const struct firmware **firmware_p, __module_get(THIS_MODULE); ret = _request_firmware(firmware_p, name, device, NULL, 0, - FW_OPT_UEVENT | FW_OPT_NO_WARN); + FW_OPT_UEVENT | FW_OPT_NO_WARN, + firmware_loading_timeout()); module_put(THIS_MODULE); return ret; } @@ -1280,7 +1280,8 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name, __module_get(THIS_MODULE); ret = _request_firmware(firmware_p, name, device, buf, size, FW_OPT_UEVENT | FW_OPT_FALLBACK | - FW_OPT_NOCACHE); + FW_OPT_NOCACHE, + firmware_loading_timeout()); module_put(THIS_MODULE); return ret; } @@ -1319,7 +1320,7 @@ static void request_firmware_work_func(struct work_struct *work) fw_work = container_of(work, struct firmware_work, work); _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, - fw_work->opt_flags); + fw_work->opt_flags, firmware_loading_timeout()); fw_work->cont(fw, fw_work->context); put_device(fw_work->device); /* taken in request_firmware_nowait() */ @@ -1391,6 +1392,16 @@ EXPORT_SYMBOL(request_firmware_nowait); #ifdef CONFIG_PM_SLEEP static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain); +/* + * use small loading timeout for caching devices' firmware + * because all these firmware images have been loaded + * successfully at lease once, also system is ready for + * completing firmware loading now. The maximum size of + * firmware in current distributions is about 2M bytes, + * so 10 secs should be enough. + */ +#define FW_LOAD_CACHED_TIMEOUT (10 * HZ) + /** * cache_firmware - cache one firmware image in kernel memory space * @fw_name: the firmware image name @@ -1412,7 +1423,11 @@ static int cache_firmware(const char *fw_name) pr_debug("%s: %s\n", __func__, fw_name); - ret = request_firmware(&fw, fw_name, NULL); + __module_get(THIS_MODULE); + ret = _request_firmware(&fw, fw_name, NULL, NULL, 0, + FW_OPT_UEVENT | FW_OPT_FALLBACK, + FW_LOAD_CACHED_TIMEOUT); + module_put(THIS_MODULE); if (!ret) kfree(fw); @@ -1622,7 +1637,6 @@ static void __device_uncache_fw_images(void) static void device_cache_fw_images(void) { struct firmware_cache *fwc = &fw_cache; - int old_timeout; DEFINE_WAIT(wait); pr_debug("%s\n", __func__); @@ -1630,17 +1644,6 @@ static void device_cache_fw_images(void) /* cancel uncache work */ cancel_delayed_work_sync(&fwc->work); - /* - * use small loading timeout for caching devices' firmware - * because all these firmware images have been loaded - * successfully at lease once, also system is ready for - * completing firmware loading now. The maximum size of - * firmware in current distributions is about 2M bytes, - * so 10 secs should be enough. - */ - old_timeout = loading_timeout; - loading_timeout = 10; - mutex_lock(&fw_lock); fwc->state = FW_LOADER_START_CACHE; dpm_for_each_dev(NULL, dev_cache_fw_image); @@ -1648,8 +1651,6 @@ static void device_cache_fw_images(void) /* wait for completion of caching firmware for all devices */ async_synchronize_full_domain(&fw_cache_domain); - - loading_timeout = old_timeout; } /**
| |