| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Aug 2016 11:50:45 +0200
From: Daniel Wagner <daniel.wagner@...-carit.de>
To: "Luis R. Rodriguez" <mcgrof@...nel.org>,
Daniel Wagner <wagi@...om.org>
CC: <linux-kernel@...r.kernel.org>, Ming Lei <ming.lei@...onical.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading
status
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;
}
/**
Powered by blists - more mailing lists