Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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;
 }
 
 /**