[PATCH 0/4] firmware: Make user-mode helper optional (v3)

[PATCH 0/4] firmware: Make user-mode helper optional (v3)

[Takashi Iwai]

Hi Ming,

here is the revised patch series after your review.
In addition to points you suggested, I changed the return value of
_request_firmware_prepare() to be more naturally processed, and fixed
one forgotten unbalanced put_device() call.
The rest are same as the former version, just adapted the revised
first patch.


thanks,

Takashi

[PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

Since 3.7 kernel, the firmware loader can read the firmware files
directly, and the traditional user-mode helper is invoked only as a
fallback.  This seems working pretty well, and the next step would be
to reduce the redundant user-mode helper stuff in future.

This patch is a preparation for that: refactor the code for splitting
user-mode helper stuff more easily.  No functional change.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 290 +++++++++++++++++++++++-------------------
 1 file changed, 157 insertions(+), 133 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b392b35..27f2c7c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
 	return true;
 }
 
-static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
+static bool fw_get_filesystem_firmware(struct device *device,
+				       struct firmware_buf *buf)
 {
 	int i;
 	bool success = false;
@@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
 			break;
 	}
 	__putname(path);
+
+	if (success) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+			buf->fw_id);
+		mutex_lock(&fw_lock);
+		set_bit(FW_STATUS_DONE, &buf->status);
+		complete_all(&buf->completion);
+		mutex_unlock(&fw_lock);
+	}
+
 	return success;
 }
 
@@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static void _request_firmware_cleanup(const struct firmware **firmware_p)
+/* wait until the shared firmware_buf becomes ready (or error) */
+static int sync_cached_firmware_buf(struct firmware_buf *buf)
 {
-	release_firmware(*firmware_p);
-	*firmware_p = NULL;
+	int ret = 0;
+
+	mutex_lock(&fw_lock);
+	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
+		if (test_bit(FW_STATUS_ABORT, &buf->status)) {
+			ret = -ENOENT;
+			break;
+		}
+		mutex_unlock(&fw_lock);
+		wait_for_completion(&buf->completion);
+		mutex_lock(&fw_lock);
+	}
+	mutex_unlock(&fw_lock);
+	return ret;
 }
 
-static struct firmware_priv *
-_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
-			  struct device *device, bool uevent, bool nowait)
+/* prepare firmware and firmware_buf structs;
+ * return 0 if a firmware is already assigned, 1 if need to load one,
+ * or a negative error code
+ */
+static int
+_request_firmware_prepare(struct firmware **firmware_p, const char *name,
+			  struct device *device)
 {
 	struct firmware *firmware;
-	struct firmware_priv *fw_priv = NULL;
 	struct firmware_buf *buf;
 	int ret;
 
-	if (!firmware_p)
-		return ERR_PTR(-EINVAL);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
 			__func__);
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
-		return NULL;
+		return 0; /* assigned */
 	}
 
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
-	if (!ret)
-		fw_priv = fw_create_instance(firmware, name, device,
-				uevent, nowait);
 
-	if (IS_ERR(fw_priv) || ret < 0) {
-		kfree(firmware);
-		*firmware_p = NULL;
-		return ERR_PTR(-ENOMEM);
-	} else if (fw_priv) {
-		fw_priv->buf = buf;
+	/*
+	 * bind with 'buf' now to avoid warning in failure path
+	 * of requesting firmware.
+	 */
+	firmware->priv = buf;
 
-		/*
-		 * bind with 'buf' now to avoid warning in failure path
-		 * of requesting firmware.
-		 */
-		firmware->priv = buf;
-		return fw_priv;
+	if (ret > 0) {
+		ret = sync_cached_firmware_buf(buf);
+		if (!ret) {
+			fw_set_page_data(buf, firmware);
+			return 0; /* assigned */
+		}
 	}
 
-	/* share the cached buf, which is inprogessing or completed */
- check_status:
+	if (ret < 0)
+		return ret;
+	return 1; /* need to load */
+}
+
+static int assign_firmware_buf(struct firmware *fw, struct device *device)
+{
+	struct firmware_buf *buf = fw->priv;
+
 	mutex_lock(&fw_lock);
-	if (test_bit(FW_STATUS_ABORT, &buf->status)) {
-		fw_priv = ERR_PTR(-ENOENT);
-		firmware->priv = buf;
-		_request_firmware_cleanup(firmware_p);
-		goto exit;
-	} else if (test_bit(FW_STATUS_DONE, &buf->status)) {
-		fw_priv = NULL;
-		fw_set_page_data(buf, firmware);
-		goto exit;
+	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
 	}
-	mutex_unlock(&fw_lock);
-	wait_for_completion(&buf->completion);
-	goto check_status;
 
-exit:
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	if (device)
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
 	mutex_unlock(&fw_lock);
-	return fw_priv;
+	return 0;
 }
 
+/* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
 	struct firmware_buf *buf = fw_priv->buf;
-	struct firmware_cache *fwc = &fw_cache;
-	int direct_load = 0;
-
-	/* try direct loading from fs first */
-	if (fw_get_filesystem_firmware(buf)) {
-		dev_dbg(f_dev->parent, "firmware: direct-loading"
-			" firmware %s\n", buf->fw_id);
-
-		mutex_lock(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		mutex_unlock(&fw_lock);
-		complete_all(&buf->completion);
-		direct_load = 1;
-		goto handle_fw;
-	}
 
 	/* fall back on userspace loading */
 	buf->fmt = PAGE_BUF;
@@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 
-handle_fw:
-	mutex_lock(&fw_lock);
-	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
-		retval = -ENOENT;
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * f_dev->parent may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	if (!retval && f_dev->parent)
-		fw_add_devm_name(f_dev->parent, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!retval && fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw_priv->fw);
-
 	fw_priv->buf = NULL;
-	mutex_unlock(&fw_lock);
-
-	if (direct_load)
-		goto err_put_dev;
 
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
@@ -972,6 +965,77 @@ err_put_dev:
 	return retval;
 }
 
+static int fw_load_from_user_helper(struct firmware *firmware,
+				    const char *name, struct device *device,
+				    bool uevent, bool nowait)
+{
+	struct firmware_priv *fw_priv;
+	long timeout;
+	int ret;
+
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv))
+		return PTR_ERR(fw_priv);
+
+	fw_priv->buf = firmware->priv;
+
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			kfree(fw_priv);
+			return -EAGAIN;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			kfree(fw_priv);
+			return ret;
+		}
+	}
+
+	ret = _request_firmware_load(fw_priv, uevent, timeout);
+	usermodehelper_read_unlock();
+	return ret;
+}
+
+/* called from request_firmware() and request_firmware_work_func() */
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+		  struct device *device, bool uevent, bool nowait)
+{
+	struct firmware *fw;
+	int ret;
+
+	if (!firmware_p)
+		return -EINVAL;
+
+	ret = _request_firmware_prepare(&fw, name, device);
+	if (ret <= 0) /* error or already assigned */
+		goto out;
+
+	ret = 0;
+	if (!fw_get_filesystem_firmware(device, fw->priv))
+		ret = fw_load_from_user_helper(fw, name, device,
+					       uevent, nowait);
+
+	if (!ret)
+		ret = assign_firmware_buf(fw, device);
+
+ out:
+	if (ret < 0) {
+		release_firmware(fw);
+		fw = NULL;
+	}
+
+	*firmware_p = fw;
+	return ret;
+}
+
 /**
  * request_firmware: - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
@@ -996,26 +1060,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	struct firmware_priv *fw_priv;
-	int ret;
-
-	fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
-					    false);
-	if (IS_ERR_OR_NULL(fw_priv))
-		return PTR_RET(fw_priv);
-
-	ret = usermodehelper_read_trylock();
-	if (WARN_ON(ret)) {
-		dev_err(device, "firmware: %s will not be loaded\n", name);
-	} else {
-		ret = _request_firmware_load(fw_priv, true,
-					firmware_loading_timeout());
-		usermodehelper_read_unlock();
-	}
-	if (ret)
-		_request_firmware_cleanup(firmware_p);
-
-	return ret;
+	return _request_firmware(firmware_p, name, device, true, false);
 }
 
 /**
@@ -1046,33 +1091,12 @@ static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
-	struct firmware_priv *fw_priv;
-	long timeout;
-	int ret;
 
 	fw_work = container_of(work, struct firmware_work, work);
-	fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
-			fw_work->uevent, true);
-	if (IS_ERR_OR_NULL(fw_priv)) {
-		ret = PTR_RET(fw_priv);
-		goto out;
-	}
 
-	timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
-	if (timeout) {
-		ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
-		usermodehelper_read_unlock();
-	} else {
-		dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
-			fw_work->name);
-		ret = -EAGAIN;
-	}
-	if (ret)
-		_request_firmware_cleanup(&fw);
-
- out:
+	_request_firmware(&fw, fw_work->name, fw_work->device,
+			  fw_work->uevent, true);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device);
 
 	module_put(fw_work->module);
 	kfree(fw_work);
-- 
1.8.1.1

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Tue, Jan 29, 2013 at 10:46 PM, Takashi Iwai <tiwai@suse.de> wrote:
> Since 3.7 kernel, the firmware loader can read the firmware files
> directly, and the traditional user-mode helper is invoked only as a
> fallback.  This seems working pretty well, and the next step would be
> to reduce the redundant user-mode helper stuff in future.
>
> This patch is a preparation for that: refactor the code for splitting
> user-mode helper stuff more easily.  No functional change.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 290 +++++++++++++++++++++++-------------------
>  1 file changed, 157 insertions(+), 133 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b392b35..27f2c7c 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>         return true;
>  }
>
> -static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> +static bool fw_get_filesystem_firmware(struct device *device,
> +                                      struct firmware_buf *buf)
>  {
>         int i;
>         bool success = false;
> @@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
>                         break;
>         }
>         __putname(path);
> +
> +       if (success) {
> +               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> +                       buf->fw_id);
> +               mutex_lock(&fw_lock);
> +               set_bit(FW_STATUS_DONE, &buf->status);
> +               complete_all(&buf->completion);
> +               mutex_unlock(&fw_lock);
> +       }
> +
>         return success;
>  }
>
> @@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
>  }
>  #endif
>
> -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> +/* wait until the shared firmware_buf becomes ready (or error) */
> +static int sync_cached_firmware_buf(struct firmware_buf *buf)
>  {
> -       release_firmware(*firmware_p);
> -       *firmware_p = NULL;
> +       int ret = 0;
> +
> +       mutex_lock(&fw_lock);
> +       while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> +               if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> +                       ret = -ENOENT;
> +                       break;
> +               }
> +               mutex_unlock(&fw_lock);
> +               wait_for_completion(&buf->completion);
> +               mutex_lock(&fw_lock);
> +       }
> +       mutex_unlock(&fw_lock);
> +       return ret;
>  }
>
> -static struct firmware_priv *
> -_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
> -                         struct device *device, bool uevent, bool nowait)
> +/* prepare firmware and firmware_buf structs;
> + * return 0 if a firmware is already assigned, 1 if need to load one,
> + * or a negative error code
> + */
> +static int
> +_request_firmware_prepare(struct firmware **firmware_p, const char *name,
> +                         struct device *device)
>  {
>         struct firmware *firmware;
> -       struct firmware_priv *fw_priv = NULL;
>         struct firmware_buf *buf;
>         int ret;
>
> -       if (!firmware_p)
> -               return ERR_PTR(-EINVAL);
> -
>         *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
>         if (!firmware) {
>                 dev_err(device, "%s: kmalloc(struct firmware) failed\n",
>                         __func__);
> -               return ERR_PTR(-ENOMEM);
> +               return -ENOMEM;
>         }
>
>         if (fw_get_builtin_firmware(firmware, name)) {
>                 dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> -               return NULL;
> +               return 0; /* assigned */
>         }
>
>         ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
> -       if (!ret)
> -               fw_priv = fw_create_instance(firmware, name, device,
> -                               uevent, nowait);
>
> -       if (IS_ERR(fw_priv) || ret < 0) {
> -               kfree(firmware);
> -               *firmware_p = NULL;
> -               return ERR_PTR(-ENOMEM);
> -       } else if (fw_priv) {
> -               fw_priv->buf = buf;
> +       /*
> +        * bind with 'buf' now to avoid warning in failure path
> +        * of requesting firmware.
> +        */
> +       firmware->priv = buf;
>
> -               /*
> -                * bind with 'buf' now to avoid warning in failure path
> -                * of requesting firmware.
> -                */
> -               firmware->priv = buf;
> -               return fw_priv;
> +       if (ret > 0) {
> +               ret = sync_cached_firmware_buf(buf);
> +               if (!ret) {
> +                       fw_set_page_data(buf, firmware);
> +                       return 0; /* assigned */
> +               }
>         }
>
> -       /* share the cached buf, which is inprogessing or completed */
> - check_status:
> +       if (ret < 0)
> +               return ret;
> +       return 1; /* need to load */
> +}
> +
> +static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +{
> +       struct firmware_buf *buf = fw->priv;
> +
>         mutex_lock(&fw_lock);
> -       if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> -               fw_priv = ERR_PTR(-ENOENT);
> -               firmware->priv = buf;
> -               _request_firmware_cleanup(firmware_p);
> -               goto exit;
> -       } else if (test_bit(FW_STATUS_DONE, &buf->status)) {
> -               fw_priv = NULL;
> -               fw_set_page_data(buf, firmware);
> -               goto exit;
> +       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
> +               mutex_unlock(&fw_lock);
> +               return -ENOENT;
>         }
> -       mutex_unlock(&fw_lock);
> -       wait_for_completion(&buf->completion);
> -       goto check_status;
>
> -exit:
> +       /*
> +        * add firmware name into devres list so that we can auto cache
> +        * and uncache firmware for device.
> +        *
> +        * device may has been deleted already, but the problem
> +        * should be fixed in devres or driver core.
> +        */
> +       if (device)
> +               fw_add_devm_name(device, buf->fw_id);
> +
> +       /*
> +        * After caching firmware image is started, let it piggyback
> +        * on request firmware.
> +        */
> +       if (buf->fwc->state == FW_LOADER_START_CACHE) {
> +               if (fw_cache_piggyback_on_request(buf->fw_id))
> +                       kref_get(&buf->ref);
> +       }
> +
> +       /* pass the pages buffer to driver at the last minute */
> +       fw_set_page_data(buf, fw);
>         mutex_unlock(&fw_lock);
> -       return fw_priv;
> +       return 0;
>  }
>
> +/* load a firmware via user helper */
>  static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                                   long timeout)
>  {
>         int retval = 0;
>         struct device *f_dev = &fw_priv->dev;
>         struct firmware_buf *buf = fw_priv->buf;
> -       struct firmware_cache *fwc = &fw_cache;
> -       int direct_load = 0;
> -
> -       /* try direct loading from fs first */
> -       if (fw_get_filesystem_firmware(buf)) {
> -               dev_dbg(f_dev->parent, "firmware: direct-loading"
> -                       " firmware %s\n", buf->fw_id);
> -
> -               mutex_lock(&fw_lock);
> -               set_bit(FW_STATUS_DONE, &buf->status);
> -               mutex_unlock(&fw_lock);
> -               complete_all(&buf->completion);
> -               direct_load = 1;
> -               goto handle_fw;
> -       }
>
>         /* fall back on userspace loading */
>         buf->fmt = PAGE_BUF;
> @@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>
>         cancel_delayed_work_sync(&fw_priv->timeout_work);
>
> -handle_fw:
> -       mutex_lock(&fw_lock);
> -       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> -               retval = -ENOENT;
> -
> -       /*
> -        * add firmware name into devres list so that we can auto cache
> -        * and uncache firmware for device.
> -        *
> -        * f_dev->parent may has been deleted already, but the problem
> -        * should be fixed in devres or driver core.
> -        */
> -       if (!retval && f_dev->parent)
> -               fw_add_devm_name(f_dev->parent, buf->fw_id);
> -
> -       /*
> -        * After caching firmware image is started, let it piggyback
> -        * on request firmware.
> -        */
> -       if (!retval && fwc->state == FW_LOADER_START_CACHE) {
> -               if (fw_cache_piggyback_on_request(buf->fw_id))
> -                       kref_get(&buf->ref);
> -       }
> -
> -       /* pass the pages buffer to driver at the last minute */
> -       fw_set_page_data(buf, fw_priv->fw);
> -
>         fw_priv->buf = NULL;
> -       mutex_unlock(&fw_lock);
> -
> -       if (direct_load)
> -               goto err_put_dev;
>
>         device_remove_file(f_dev, &dev_attr_loading);
>  err_del_bin_attr:
> @@ -972,6 +965,77 @@ err_put_dev:
>         return retval;
>  }
>
> +static int fw_load_from_user_helper(struct firmware *firmware,
> +                                   const char *name, struct device *device,
> +                                   bool uevent, bool nowait)
> +{
> +       struct firmware_priv *fw_priv;
> +       long timeout;
> +       int ret;
> +
> +       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> +       if (IS_ERR(fw_priv))
> +               return PTR_ERR(fw_priv);
> +
> +       fw_priv->buf = firmware->priv;
> +
> +       timeout = firmware_loading_timeout();
> +       if (nowait) {
> +               timeout = usermodehelper_read_lock_wait(timeout);
> +               if (!timeout) {
> +                       dev_dbg(device, "firmware: %s loading timed out\n",
> +                               name);
> +                       kfree(fw_priv);
> +                       return -EAGAIN;
> +               }
> +       } else {
> +               ret = usermodehelper_read_trylock();
> +               if (WARN_ON(ret)) {
> +                       dev_err(device, "firmware: %s will not be loaded\n",
> +                               name);
> +                       kfree(fw_priv);
> +                       return ret;
> +               }
> +       }

The above usermodehelper_read_lock thing may be a functional change,
and looks not what you claimed in commit log, :-). The lock is currently held in
direct loading case, but your patch change the rule. Without holding the lock,
request_firmware() may touch filesystem / storage too early during
kernel boot or system resume in direct loading case.

> +
> +       ret = _request_firmware_load(fw_priv, uevent, timeout);
> +       usermodehelper_read_unlock();
> +       return ret;
> +}
> +
> +/* called from request_firmware() and request_firmware_work_func() */
> +static int
> +_request_firmware(const struct firmware **firmware_p, const char *name,
> +                 struct device *device, bool uevent, bool nowait)
> +{
> +       struct firmware *fw;
> +       int ret;
> +
> +       if (!firmware_p)
> +               return -EINVAL;
> +
> +       ret = _request_firmware_prepare(&fw, name, device);
> +       if (ret <= 0) /* error or already assigned */
> +               goto out;
> +
> +       ret = 0;
> +       if (!fw_get_filesystem_firmware(device, fw->priv))
> +               ret = fw_load_from_user_helper(fw, name, device,
> +                                              uevent, nowait);
> +
> +       if (!ret)
> +               ret = assign_firmware_buf(fw, device);
> +
> + out:
> +       if (ret < 0) {
> +               release_firmware(fw);
> +               fw = NULL;
> +       }
> +
> +       *firmware_p = fw;
> +       return ret;
> +}
> +
>  /**
>   * request_firmware: - send firmware request and wait for it
>   * @firmware_p: pointer to firmware image
> @@ -996,26 +1060,7 @@ int
>  request_firmware(const struct firmware **firmware_p, const char *name,
>                   struct device *device)
>  {
> -       struct firmware_priv *fw_priv;
> -       int ret;
> -
> -       fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
> -                                           false);
> -       if (IS_ERR_OR_NULL(fw_priv))
> -               return PTR_RET(fw_priv);
> -
> -       ret = usermodehelper_read_trylock();
> -       if (WARN_ON(ret)) {
> -               dev_err(device, "firmware: %s will not be loaded\n", name);
> -       } else {
> -               ret = _request_firmware_load(fw_priv, true,
> -                                       firmware_loading_timeout());
> -               usermodehelper_read_unlock();
> -       }
> -       if (ret)
> -               _request_firmware_cleanup(firmware_p);
> -
> -       return ret;
> +       return _request_firmware(firmware_p, name, device, true, false);
>  }
>
>  /**
> @@ -1046,33 +1091,12 @@ static void request_firmware_work_func(struct work_struct *work)
>  {
>         struct firmware_work *fw_work;
>         const struct firmware *fw;
> -       struct firmware_priv *fw_priv;
> -       long timeout;
> -       int ret;
>
>         fw_work = container_of(work, struct firmware_work, work);
> -       fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
> -                       fw_work->uevent, true);
> -       if (IS_ERR_OR_NULL(fw_priv)) {
> -               ret = PTR_RET(fw_priv);
> -               goto out;
> -       }
>
> -       timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
> -       if (timeout) {
> -               ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
> -               usermodehelper_read_unlock();
> -       } else {
> -               dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
> -                       fw_work->name);
> -               ret = -EAGAIN;
> -       }
> -       if (ret)
> -               _request_firmware_cleanup(&fw);
> -
> - out:
> +       _request_firmware(&fw, fw_work->name, fw_work->device,
> +                         fw_work->uevent, true);
>         fw_work->cont(fw, fw_work->context);
> -       put_device(fw_work->device);

The above put_device is the pair of get_device inside request_firmware_nowait(),
I am wondering why you think it is not balanced, and to be removed . Did you
observe a double free?

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Wed, 30 Jan 2013 11:37:30 +0800,
Ming Lei wrote:
> 
> On Tue, Jan 29, 2013 at 10:46 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > Since 3.7 kernel, the firmware loader can read the firmware files
> > directly, and the traditional user-mode helper is invoked only as a
> > fallback.  This seems working pretty well, and the next step would be
> > to reduce the redundant user-mode helper stuff in future.
> >
> > This patch is a preparation for that: refactor the code for splitting
> > user-mode helper stuff more easily.  No functional change.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 290 +++++++++++++++++++++++-------------------
> >  1 file changed, 157 insertions(+), 133 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index b392b35..27f2c7c 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
> >         return true;
> >  }
> >
> > -static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> > +static bool fw_get_filesystem_firmware(struct device *device,
> > +                                      struct firmware_buf *buf)
> >  {
> >         int i;
> >         bool success = false;
> > @@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> >                         break;
> >         }
> >         __putname(path);
> > +
> > +       if (success) {
> > +               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> > +                       buf->fw_id);
> > +               mutex_lock(&fw_lock);
> > +               set_bit(FW_STATUS_DONE, &buf->status);
> > +               complete_all(&buf->completion);
> > +               mutex_unlock(&fw_lock);
> > +       }
> > +
> >         return success;
> >  }
> >
> > @@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
> >  }
> >  #endif
> >
> > -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> > +/* wait until the shared firmware_buf becomes ready (or error) */
> > +static int sync_cached_firmware_buf(struct firmware_buf *buf)
> >  {
> > -       release_firmware(*firmware_p);
> > -       *firmware_p = NULL;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&fw_lock);
> > +       while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> > +               if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> > +                       ret = -ENOENT;
> > +                       break;
> > +               }
> > +               mutex_unlock(&fw_lock);
> > +               wait_for_completion(&buf->completion);
> > +               mutex_lock(&fw_lock);
> > +       }
> > +       mutex_unlock(&fw_lock);
> > +       return ret;
> >  }
> >
> > -static struct firmware_priv *
> > -_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
> > -                         struct device *device, bool uevent, bool nowait)
> > +/* prepare firmware and firmware_buf structs;
> > + * return 0 if a firmware is already assigned, 1 if need to load one,
> > + * or a negative error code
> > + */
> > +static int
> > +_request_firmware_prepare(struct firmware **firmware_p, const char *name,
> > +                         struct device *device)
> >  {
> >         struct firmware *firmware;
> > -       struct firmware_priv *fw_priv = NULL;
> >         struct firmware_buf *buf;
> >         int ret;
> >
> > -       if (!firmware_p)
> > -               return ERR_PTR(-EINVAL);
> > -
> >         *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
> >         if (!firmware) {
> >                 dev_err(device, "%s: kmalloc(struct firmware) failed\n",
> >                         __func__);
> > -               return ERR_PTR(-ENOMEM);
> > +               return -ENOMEM;
> >         }
> >
> >         if (fw_get_builtin_firmware(firmware, name)) {
> >                 dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> > -               return NULL;
> > +               return 0; /* assigned */
> >         }
> >
> >         ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
> > -       if (!ret)
> > -               fw_priv = fw_create_instance(firmware, name, device,
> > -                               uevent, nowait);
> >
> > -       if (IS_ERR(fw_priv) || ret < 0) {
> > -               kfree(firmware);
> > -               *firmware_p = NULL;
> > -               return ERR_PTR(-ENOMEM);
> > -       } else if (fw_priv) {
> > -               fw_priv->buf = buf;
> > +       /*
> > +        * bind with 'buf' now to avoid warning in failure path
> > +        * of requesting firmware.
> > +        */
> > +       firmware->priv = buf;
> >
> > -               /*
> > -                * bind with 'buf' now to avoid warning in failure path
> > -                * of requesting firmware.
> > -                */
> > -               firmware->priv = buf;
> > -               return fw_priv;
> > +       if (ret > 0) {
> > +               ret = sync_cached_firmware_buf(buf);
> > +               if (!ret) {
> > +                       fw_set_page_data(buf, firmware);
> > +                       return 0; /* assigned */
> > +               }
> >         }
> >
> > -       /* share the cached buf, which is inprogessing or completed */
> > - check_status:
> > +       if (ret < 0)
> > +               return ret;
> > +       return 1; /* need to load */
> > +}
> > +
> > +static int assign_firmware_buf(struct firmware *fw, struct device *device)
> > +{
> > +       struct firmware_buf *buf = fw->priv;
> > +
> >         mutex_lock(&fw_lock);
> > -       if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> > -               fw_priv = ERR_PTR(-ENOENT);
> > -               firmware->priv = buf;
> > -               _request_firmware_cleanup(firmware_p);
> > -               goto exit;
> > -       } else if (test_bit(FW_STATUS_DONE, &buf->status)) {
> > -               fw_priv = NULL;
> > -               fw_set_page_data(buf, firmware);
> > -               goto exit;
> > +       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
> > +               mutex_unlock(&fw_lock);
> > +               return -ENOENT;
> >         }
> > -       mutex_unlock(&fw_lock);
> > -       wait_for_completion(&buf->completion);
> > -       goto check_status;
> >
> > -exit:
> > +       /*
> > +        * add firmware name into devres list so that we can auto cache
> > +        * and uncache firmware for device.
> > +        *
> > +        * device may has been deleted already, but the problem
> > +        * should be fixed in devres or driver core.
> > +        */
> > +       if (device)
> > +               fw_add_devm_name(device, buf->fw_id);
> > +
> > +       /*
> > +        * After caching firmware image is started, let it piggyback
> > +        * on request firmware.
> > +        */
> > +       if (buf->fwc->state == FW_LOADER_START_CACHE) {
> > +               if (fw_cache_piggyback_on_request(buf->fw_id))
> > +                       kref_get(&buf->ref);
> > +       }
> > +
> > +       /* pass the pages buffer to driver at the last minute */
> > +       fw_set_page_data(buf, fw);
> >         mutex_unlock(&fw_lock);
> > -       return fw_priv;
> > +       return 0;
> >  }
> >
> > +/* load a firmware via user helper */
> >  static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> >                                   long timeout)
> >  {
> >         int retval = 0;
> >         struct device *f_dev = &fw_priv->dev;
> >         struct firmware_buf *buf = fw_priv->buf;
> > -       struct firmware_cache *fwc = &fw_cache;
> > -       int direct_load = 0;
> > -
> > -       /* try direct loading from fs first */
> > -       if (fw_get_filesystem_firmware(buf)) {
> > -               dev_dbg(f_dev->parent, "firmware: direct-loading"
> > -                       " firmware %s\n", buf->fw_id);
> > -
> > -               mutex_lock(&fw_lock);
> > -               set_bit(FW_STATUS_DONE, &buf->status);
> > -               mutex_unlock(&fw_lock);
> > -               complete_all(&buf->completion);
> > -               direct_load = 1;
> > -               goto handle_fw;
> > -       }
> >
> >         /* fall back on userspace loading */
> >         buf->fmt = PAGE_BUF;
> > @@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> >
> >         cancel_delayed_work_sync(&fw_priv->timeout_work);
> >
> > -handle_fw:
> > -       mutex_lock(&fw_lock);
> > -       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> > -               retval = -ENOENT;
> > -
> > -       /*
> > -        * add firmware name into devres list so that we can auto cache
> > -        * and uncache firmware for device.
> > -        *
> > -        * f_dev->parent may has been deleted already, but the problem
> > -        * should be fixed in devres or driver core.
> > -        */
> > -       if (!retval && f_dev->parent)
> > -               fw_add_devm_name(f_dev->parent, buf->fw_id);
> > -
> > -       /*
> > -        * After caching firmware image is started, let it piggyback
> > -        * on request firmware.
> > -        */
> > -       if (!retval && fwc->state == FW_LOADER_START_CACHE) {
> > -               if (fw_cache_piggyback_on_request(buf->fw_id))
> > -                       kref_get(&buf->ref);
> > -       }
> > -
> > -       /* pass the pages buffer to driver at the last minute */
> > -       fw_set_page_data(buf, fw_priv->fw);
> > -
> >         fw_priv->buf = NULL;
> > -       mutex_unlock(&fw_lock);
> > -
> > -       if (direct_load)
> > -               goto err_put_dev;
> >
> >         device_remove_file(f_dev, &dev_attr_loading);
> >  err_del_bin_attr:
> > @@ -972,6 +965,77 @@ err_put_dev:
> >         return retval;
> >  }
> >
> > +static int fw_load_from_user_helper(struct firmware *firmware,
> > +                                   const char *name, struct device *device,
> > +                                   bool uevent, bool nowait)
> > +{
> > +       struct firmware_priv *fw_priv;
> > +       long timeout;
> > +       int ret;
> > +
> > +       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> > +       if (IS_ERR(fw_priv))
> > +               return PTR_ERR(fw_priv);
> > +
> > +       fw_priv->buf = firmware->priv;
> > +
> > +       timeout = firmware_loading_timeout();
> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       kfree(fw_priv);
> > +                       return -EAGAIN;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +                       kfree(fw_priv);
> > +                       return ret;
> > +               }
> > +       }
> 
> The above usermodehelper_read_lock thing may be a functional change,
> and looks not what you claimed in commit log, :-). The lock is currently held in
> direct loading case, but your patch change the rule. Without holding the lock,
> request_firmware() may touch filesystem / storage too early during
> kernel boot or system resume in direct loading case.

Does it really happen in a real scenario?

If so, using usermode helper lock for that purpose sounds like an
abuse to be fixed differently or replaced with a better one.


> > +
> > +       ret = _request_firmware_load(fw_priv, uevent, timeout);
> > +       usermodehelper_read_unlock();
> > +       return ret;
> > +}
> > +
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       int ret;
> > +
> > +       if (!firmware_p)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret <= 0) /* error or already assigned */
> > +               goto out;
> > +
> > +       ret = 0;
> > +       if (!fw_get_filesystem_firmware(device, fw->priv))
> > +               ret = fw_load_from_user_helper(fw, name, device,
> > +                                              uevent, nowait);
> > +
> > +       if (!ret)
> > +               ret = assign_firmware_buf(fw, device);
> > +
> > + out:
> > +       if (ret < 0) {
> > +               release_firmware(fw);
> > +               fw = NULL;
> > +       }
> > +
> > +       *firmware_p = fw;
> > +       return ret;
> > +}
> > +
> >  /**
> >   * request_firmware: - send firmware request and wait for it
> >   * @firmware_p: pointer to firmware image
> > @@ -996,26 +1060,7 @@ int
> >  request_firmware(const struct firmware **firmware_p, const char *name,
> >                   struct device *device)
> >  {
> > -       struct firmware_priv *fw_priv;
> > -       int ret;
> > -
> > -       fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
> > -                                           false);
> > -       if (IS_ERR_OR_NULL(fw_priv))
> > -               return PTR_RET(fw_priv);
> > -
> > -       ret = usermodehelper_read_trylock();
> > -       if (WARN_ON(ret)) {
> > -               dev_err(device, "firmware: %s will not be loaded\n", name);
> > -       } else {
> > -               ret = _request_firmware_load(fw_priv, true,
> > -                                       firmware_loading_timeout());
> > -               usermodehelper_read_unlock();
> > -       }
> > -       if (ret)
> > -               _request_firmware_cleanup(firmware_p);
> > -
> > -       return ret;
> > +       return _request_firmware(firmware_p, name, device, true, false);
> >  }
> >
> >  /**
> > @@ -1046,33 +1091,12 @@ static void request_firmware_work_func(struct work_struct *work)
> >  {
> >         struct firmware_work *fw_work;
> >         const struct firmware *fw;
> > -       struct firmware_priv *fw_priv;
> > -       long timeout;
> > -       int ret;
> >
> >         fw_work = container_of(work, struct firmware_work, work);
> > -       fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
> > -                       fw_work->uevent, true);
> > -       if (IS_ERR_OR_NULL(fw_priv)) {
> > -               ret = PTR_RET(fw_priv);
> > -               goto out;
> > -       }
> >
> > -       timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
> > -       if (timeout) {
> > -               ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
> > -               usermodehelper_read_unlock();
> > -       } else {
> > -               dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
> > -                       fw_work->name);
> > -               ret = -EAGAIN;
> > -       }
> > -       if (ret)
> > -               _request_firmware_cleanup(&fw);
> > -
> > - out:
> > +       _request_firmware(&fw, fw_work->name, fw_work->device,
> > +                         fw_work->uevent, true);
> >         fw_work->cont(fw, fw_work->context);
> > -       put_device(fw_work->device);
> 
> The above put_device is the pair of get_device inside request_firmware_nowait(),
> I am wondering why you think it is not balanced, and to be removed . Did you
> observe a double free?

Oh yeah, I completely misread the code.  It should remain there.



Takashi

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Wed, Jan 30, 2013 at 3:17 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> The above usermodehelper_read_lock thing may be a functional change,
>> and looks not what you claimed in commit log, :-). The lock is currently held in
>> direct loading case, but your patch change the rule. Without holding the lock,
>> request_firmware() may touch filesystem / storage too early during
>> kernel boot or system resume in direct loading case.
>
> Does it really happen in a real scenario?

Some crazy drivers might call request_firmware inside resume callback,
with usermodehelper_read_lock, we can find the mistake easily and log it.

>
> If so, using usermode helper lock for that purpose sounds like an
> abuse to be fixed differently or replaced with a better one.

Might be, but looks not good to introduce this change in a code
refactoring patch. Or you can do it in another patch for discussion
if you have better way to handle the situation.


Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Wed, 30 Jan 2013 18:25:20 +0800,
Ming Lei wrote:
> 
> On Wed, Jan 30, 2013 at 3:17 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> The above usermodehelper_read_lock thing may be a functional change,
> >> and looks not what you claimed in commit log, :-). The lock is currently held in
> >> direct loading case, but your patch change the rule. Without holding the lock,
> >> request_firmware() may touch filesystem / storage too early during
> >> kernel boot or system resume in direct loading case.
> >
> > Does it really happen in a real scenario?
> 
> Some crazy drivers might call request_firmware inside resume callback,
> with usermodehelper_read_lock, we can find the mistake easily and log it.

But it's supposed to be cached, no?

> > If so, using usermode helper lock for that purpose sounds like an
> > abuse to be fixed differently or replaced with a better one.
> 
> Might be, but looks not good to introduce this change in a code
> refactoring patch. Or you can do it in another patch for discussion
> if you have better way to handle the situation.

Yes, I already modified the code again.
Will resubmit them soon.


Takashi

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>
> But it's supposed to be cached, no?

Generally it will be cached, but some crazy devices might come as new
device during resume, so we still need to handle the situation.

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Wed, 30 Jan 2013 18:50:05 +0800,
Ming Lei wrote:
> 
> On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > But it's supposed to be cached, no?
> 
> Generally it will be cached, but some crazy devices might come as new
> device during resume, so we still need to handle the situation.

In that case, shouldn't we simply return an error instead of
usermodehelper lock (at least for direct loading)?


Takashi

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Wed, Jan 30, 2013 at 6:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 30 Jan 2013 18:50:05 +0800,
> Ming Lei wrote:
>>
>> On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >
>> > But it's supposed to be cached, no?
>>
>> Generally it will be cached, but some crazy devices might come as new
>> device during resume, so we still need to handle the situation.
>
> In that case, shouldn't we simply return an error instead of
> usermodehelper lock (at least for direct loading)?

The check depends on usermodehelper_read_trylock now, is there
other simpler way to do the check?

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Wed, 30 Jan 2013 11:53:14 +0100,
Takashi Iwai wrote:
> 
> At Wed, 30 Jan 2013 18:50:05 +0800,
> Ming Lei wrote:
> > 
> > On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > But it's supposed to be cached, no?
> > 
> > Generally it will be cached, but some crazy devices might come as new
> > device during resume, so we still need to handle the situation.
> 
> In that case, shouldn't we simply return an error instead of
> usermodehelper lock (at least for direct loading)?

The patch below is what I have in my mind...


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] firmware: Skip usermodehelper_lock for direct loading

We don't need a user mode helper lock for the direct fw loading.
This also reduces the use of timeout when user mode helper is
disabled, thus we can move the code into ifdef block, too.

For avoiding the possible call of request_firmware() without caching,
add a check of suspend state for the direct loading case, and returns
immediately if it's called during suspend/resume.  Then it proceeds to
the user mode helper if enabled, or returns the error.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 97 ++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 43 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f1caad7..c973bb0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -88,13 +88,6 @@ enum {
 	FW_STATUS_ABORT,
 };
 
-static int loading_timeout = 60;	/* In seconds */
-
-static inline long firmware_loading_timeout(void)
-{
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
-}
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -315,6 +308,9 @@ static bool fw_get_filesystem_firmware(struct device *device,
 	bool success = false;
 	char *path = __getname();
 
+	if (device->power.is_suspended)
+		return false;
+
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
 
@@ -457,6 +453,20 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 #define is_fw_load_aborted(buf)	\
 	test_bit(FW_STATUS_ABORT, &(buf)->status)
 
+static int loading_timeout = 60;	/* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+}
+
+static inline long set_loading_timeout(long val)
+{
+	long old_timeout = loading_timeout;
+	loading_timeout = val;
+	return old_timeout;
+}
+
 static ssize_t firmware_timeout_show(struct class *class,
 				     struct class_attribute *attr,
 				     char *buf)
@@ -875,22 +885,44 @@ err_put_dev:
 
 static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
-				    bool uevent, bool nowait, long timeout)
+				    bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;
+	long timeout;
+	int ret;
+
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			return -EBUSY;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			return ret;
+		}
+	}
 
 	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
 	if (IS_ERR(fw_priv))
-		return PTR_ERR(fw_priv);
+		ret = PTR_ERR(fw_priv);
+	else {
+		fw_priv->buf = firmware->priv;
+		ret = _request_firmware_load(fw_priv, uevent, timeout);
+	}
 
-	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, uevent, timeout);
+	usermodehelper_read_unlock();
+	return ret;
 }
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
 fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, bool uevent, bool nowait,
-			 long timeout)
+			 struct device *device, bool uevent, bool nowait)
 {
 	return -ENOENT;
 }
@@ -898,6 +930,8 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 /* No abort during direct loading */
 #define is_fw_load_aborted(buf) false
 
+static inline long set_loading_timeout(long val) { return 0; }
+
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 
@@ -1006,7 +1040,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		  struct device *device, bool uevent, bool nowait)
 {
 	struct firmware *fw;
-	long timeout;
 	int ret;
 
 	if (!firmware_p)
@@ -1017,32 +1050,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 
 	ret = 0;
-	timeout = firmware_loading_timeout();
-	if (nowait) {
-		timeout = usermodehelper_read_lock_wait(timeout);
-		if (!timeout) {
-			dev_dbg(device, "firmware: %s loading timed out\n",
-				name);
-			ret = -EBUSY;
-		}
-	} else {
-		ret = usermodehelper_read_trylock();
-		if (WARN_ON(ret)) {
-			dev_err(device, "firmware: %s will not be loaded\n",
-				name);
-		}
-	}
-
-	if (!ret) {
-		if (!fw_get_filesystem_firmware(device, fw->priv))
-			ret = fw_load_from_user_helper(fw, name, device,
-						       uevent, nowait,
-						       timeout);
-		if (!ret)
-			ret = assign_firmware_buf(fw, device);
-	}
-
-	usermodehelper_read_unlock();
+	if (!fw_get_filesystem_firmware(device, fw->priv))
+		ret = fw_load_from_user_helper(fw, name, device,
+					       uevent, nowait);
+	if (!ret)
+		ret = assign_firmware_buf(fw, device);
 
  out:
 	if (ret < 0) {
@@ -1406,8 +1418,7 @@ static void device_cache_fw_images(void)
 	 * firmware in current distributions is about 2M bytes,
 	 * so 10 secs should be enough.
 	 */
-	old_timeout = loading_timeout;
-	loading_timeout = 10;
+	old_timeout = set_loading_timeout(10);
 
 	mutex_lock(&fw_lock);
 	fwc->state = FW_LOADER_START_CACHE;
@@ -1417,7 +1428,7 @@ 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;
+	set_loading_timeout(old_timeout);
 }
 
 /**
-- 
1.8.1.1

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Wed, Jan 30, 2013 at 7:08 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 30 Jan 2013 11:53:14 +0100,
> Takashi Iwai wrote:
>>
>> At Wed, 30 Jan 2013 18:50:05 +0800,
>> Ming Lei wrote:
>> >
>> > On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > >
>> > > But it's supposed to be cached, no?
>> >
>> > Generally it will be cached, but some crazy devices might come as new
>> > device during resume, so we still need to handle the situation.
>>
>> In that case, shouldn't we simply return an error instead of
>> usermodehelper lock (at least for direct loading)?
>
> The patch below is what I have in my mind...
>
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] firmware: Skip usermodehelper_lock for direct loading
>
> We don't need a user mode helper lock for the direct fw loading.
> This also reduces the use of timeout when user mode helper is
> disabled, thus we can move the code into ifdef block, too.
>
> For avoiding the possible call of request_firmware() without caching,
> add a check of suspend state for the direct loading case, and returns
> immediately if it's called during suspend/resume.  Then it proceeds to
> the user mode helper if enabled, or returns the error.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 97 ++++++++++++++++++++++++-------------------
>  1 file changed, 54 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index f1caad7..c973bb0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -88,13 +88,6 @@ enum {
>         FW_STATUS_ABORT,
>  };
>
> -static int loading_timeout = 60;       /* In seconds */
> -
> -static inline long firmware_loading_timeout(void)
> -{
> -       return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
> -}
> -
>  struct firmware_cache {
>         /* firmware_buf instance will be added into the below list */
>         spinlock_t lock;
> @@ -315,6 +308,9 @@ static bool fw_get_filesystem_firmware(struct device *device,
>         bool success = false;
>         char *path = __getname();
>
> +       if (device->power.is_suspended)
> +               return false;

The device which is requesting firmware is resumed  does not mean the
storage device has been resumed, so this check isn't enough.

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Wed, 30 Jan 2013 19:48:15 +0800,
Ming Lei wrote:
> 
> On Wed, Jan 30, 2013 at 7:08 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Wed, 30 Jan 2013 11:53:14 +0100,
> > Takashi Iwai wrote:
> >>
> >> At Wed, 30 Jan 2013 18:50:05 +0800,
> >> Ming Lei wrote:
> >> >
> >> > On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > >
> >> > > But it's supposed to be cached, no?
> >> >
> >> > Generally it will be cached, but some crazy devices might come as new
> >> > device during resume, so we still need to handle the situation.
> >>
> >> In that case, shouldn't we simply return an error instead of
> >> usermodehelper lock (at least for direct loading)?
> >
> > The patch below is what I have in my mind...
> >
> >
> > Takashi
> >
> > ---
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] firmware: Skip usermodehelper_lock for direct loading
> >
> > We don't need a user mode helper lock for the direct fw loading.
> > This also reduces the use of timeout when user mode helper is
> > disabled, thus we can move the code into ifdef block, too.
> >
> > For avoiding the possible call of request_firmware() without caching,
> > add a check of suspend state for the direct loading case, and returns
> > immediately if it's called during suspend/resume.  Then it proceeds to
> > the user mode helper if enabled, or returns the error.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 97 ++++++++++++++++++++++++-------------------
> >  1 file changed, 54 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index f1caad7..c973bb0 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -88,13 +88,6 @@ enum {
> >         FW_STATUS_ABORT,
> >  };
> >
> > -static int loading_timeout = 60;       /* In seconds */
> > -
> > -static inline long firmware_loading_timeout(void)
> > -{
> > -       return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
> > -}
> > -
> >  struct firmware_cache {
> >         /* firmware_buf instance will be added into the below list */
> >         spinlock_t lock;
> > @@ -315,6 +308,9 @@ static bool fw_get_filesystem_firmware(struct device *device,
> >         bool success = false;
> >         char *path = __getname();
> >
> > +       if (device->power.is_suspended)
> > +               return false;
> 
> The device which is requesting firmware is resumed  does not mean the
> storage device has been resumed, so this check isn't enough.

Well, this practically disables the call of request_firmware() in the
resume callback without caching.


Takashi

[PATCH 2/4] firmware: Make user-mode helper optional

[Takashi Iwai]

This patch adds a new kconfig, CONFIG_FW_LOADER_USER_HELPER, and
guards the user-helper codes in firmware_class.c with ifdefs.

Yeah, yeah, there are lots of ifdefs in this patch.  The further
clean-up with code shuffling follows in the next.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/Kconfig          | 11 ++++++++
 drivers/base/firmware_class.c | 62 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index c8b4539..07abd9d 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -145,6 +145,17 @@ config EXTRA_FIRMWARE_DIR
 	  this option you can point it elsewhere, such as /lib/firmware/ or
 	  some other directory containing the firmware files.
 
+config FW_LOADER_USER_HELPER
+	bool "Fallback user-helper invocation for firmware loading"
+	depends on FW_LOADER
+	default y
+	help
+	  This option enables / disables the invocation of user-helper
+	  (e.g. udev) for loading firmware files as a fallback after the
+	  direct file loading in kernel fails.  The user-mode helper is
+	  no longer required unless you have a special firmware file that
+	  resides in a non-standard path.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 27f2c7c..aa9e8606 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -88,6 +88,7 @@ enum {
 	FW_STATUS_ABORT,
 };
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 enum fw_buf_fmt {
 	VMALLOC_BUF,	/* used in direct loading */
 	PAGE_BUF,	/* used in loading via userspace */
@@ -99,6 +100,7 @@ static inline long firmware_loading_timeout(void)
 {
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
 }
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -128,12 +130,14 @@ struct firmware_buf {
 	struct completion completion;
 	struct firmware_cache *fwc;
 	unsigned long status;
-	enum fw_buf_fmt fmt;
 	void *data;
 	size_t size;
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+	enum fw_buf_fmt fmt;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
+#endif
 	char fw_id[];
 };
 
@@ -142,6 +146,7 @@ struct fw_cache_entry {
 	char name[];
 };
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 struct firmware_priv {
 	struct delayed_work timeout_work;
 	bool nowait;
@@ -149,6 +154,7 @@ struct firmware_priv {
 	struct firmware_buf *buf;
 	struct firmware *fw;
 };
+#endif
 
 struct fw_name_devm {
 	unsigned long magic;
@@ -182,7 +188,9 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	strcpy(buf->fw_id, fw_name);
 	buf->fwc = fwc;
 	init_completion(&buf->completion);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	buf->fmt = VMALLOC_BUF;
+#endif
 
 	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
 
@@ -240,7 +248,6 @@ static void __fw_free_buf(struct kref *ref)
 {
 	struct firmware_buf *buf = to_fwbuf(ref);
 	struct firmware_cache *fwc = buf->fwc;
-	int i;
 
 	pr_debug("%s: fw-%s buf=%p data=%p size=%u\n",
 		 __func__, buf->fw_id, buf, buf->data,
@@ -250,12 +257,15 @@ static void __fw_free_buf(struct kref *ref)
 	spin_unlock(&fwc->lock);
 
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	if (buf->fmt == PAGE_BUF) {
+		int i;
 		vunmap(buf->data);
 		for (i = 0; i < buf->nr_pages; i++)
 			__free_page(buf->pages[i]);
 		kfree(buf->pages);
 	} else
+#endif
 		vfree(buf->data);
 	kfree(buf);
 }
@@ -357,6 +367,19 @@ static bool fw_get_filesystem_firmware(struct device *device,
 	return success;
 }
 
+/* firmware holds the ownership of pages */
+static void firmware_free_data(const struct firmware *fw)
+{
+	/* Loaded directly? */
+	if (!fw->priv) {
+		vfree(fw->data);
+		return;
+	}
+	fw_free_buf(fw->priv);
+}
+
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
 static struct firmware_priv *to_firmware_priv(struct device *dev)
 {
 	return container_of(dev, struct firmware_priv, dev);
@@ -446,17 +469,6 @@ static ssize_t firmware_loading_show(struct device *dev,
 	return sprintf(buf, "%d\n", loading);
 }
 
-/* firmware holds the ownership of pages */
-static void firmware_free_data(const struct firmware *fw)
-{
-	/* Loaded directly? */
-	if (!fw->priv) {
-		vfree(fw->data);
-		return;
-	}
-	fw_free_buf(fw->priv);
-}
-
 /* Some architectures don't have PAGE_KERNEL_RO */
 #ifndef PAGE_KERNEL_RO
 #define PAGE_KERNEL_RO PAGE_KERNEL
@@ -737,12 +749,15 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 exit:
 	return fw_priv;
 }
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 /* store the pages buffer info firmware from buf */
 static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
 {
 	fw->priv = buf;
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	fw->pages = buf->pages;
+#endif
 	fw->size = buf->size;
 	fw->data = buf->data;
 
@@ -906,6 +921,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
 	return 0;
 }
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 /* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
@@ -1002,6 +1018,14 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	usermodehelper_read_unlock();
 	return ret;
 }
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+static inline int
+fw_load_from_user_helper(struct firmware *firmware, const char *name,
+			 struct device *device, bool uevent, bool nowait)
+{
+	return -ENOENT;
+}
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 /* called from request_firmware() and request_firmware_work_func() */
 static int
@@ -1371,7 +1395,9 @@ static void __device_uncache_fw_images(void)
 static void device_cache_fw_images(void)
 {
 	struct firmware_cache *fwc = &fw_cache;
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	int old_timeout;
+#endif
 	DEFINE_WAIT(wait);
 
 	pr_debug("%s\n", __func__);
@@ -1379,6 +1405,7 @@ static void device_cache_fw_images(void)
 	/* cancel uncache work */
 	cancel_delayed_work_sync(&fwc->work);
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	/*
 	 * use small loading timeout for caching devices' firmware
 	 * because all these firmware images have been loaded
@@ -1389,6 +1416,7 @@ static void device_cache_fw_images(void)
 	 */
 	old_timeout = loading_timeout;
 	loading_timeout = 10;
+#endif
 
 	mutex_lock(&fw_lock);
 	fwc->state = FW_LOADER_START_CACHE;
@@ -1398,7 +1426,9 @@ static void device_cache_fw_images(void)
 	/* wait for completion of caching firmware for all devices */
 	async_synchronize_full_domain(&fw_cache_domain);
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	loading_timeout = old_timeout;
+#endif
 }
 
 /**
@@ -1498,7 +1528,11 @@ static void __init fw_cache_init(void)
 static int __init firmware_class_init(void)
 {
 	fw_cache_init();
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	return class_register(&firmware_class);
+#else
+	return 0;
+#endif
 }
 
 static void __exit firmware_class_exit(void)
@@ -1507,7 +1541,9 @@ static void __exit firmware_class_exit(void)
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
 #endif
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	class_unregister(&firmware_class);
+#endif
 }
 
 fs_initcall(firmware_class_init);
-- 
1.8.1.1

[PATCH 3/4] firmware: Reduce ifdef CONFIG_FW_LOADER_USER_HELPER

[Takashi Iwai]

By shuffling the code, reduce a few ifdefs in firmware_class.c.
Also, firmware_buf fmt field is changed to is_pages_buf boolean for
simplification.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 369 ++++++++++++++++++++----------------------
 1 file changed, 179 insertions(+), 190 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index aa9e8606..8c40257 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -88,20 +88,6 @@ enum {
 	FW_STATUS_ABORT,
 };
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-enum fw_buf_fmt {
-	VMALLOC_BUF,	/* used in direct loading */
-	PAGE_BUF,	/* used in loading via userspace */
-};
-
-static int loading_timeout = 60;	/* In seconds */
-
-static inline long firmware_loading_timeout(void)
-{
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
-}
-#endif /* CONFIG_FW_LOADER_USER_HELPER */
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -133,7 +119,7 @@ struct firmware_buf {
 	void *data;
 	size_t size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-	enum fw_buf_fmt fmt;
+	bool is_paged_buf;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
@@ -146,16 +132,6 @@ struct fw_cache_entry {
 	char name[];
 };
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-struct firmware_priv {
-	struct delayed_work timeout_work;
-	bool nowait;
-	struct device dev;
-	struct firmware_buf *buf;
-	struct firmware *fw;
-};
-#endif
-
 struct fw_name_devm {
 	unsigned long magic;
 	char name[];
@@ -188,9 +164,6 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	strcpy(buf->fw_id, fw_name);
 	buf->fwc = fwc;
 	init_completion(&buf->completion);
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-	buf->fmt = VMALLOC_BUF;
-#endif
 
 	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
 
@@ -256,9 +229,8 @@ static void __fw_free_buf(struct kref *ref)
 	list_del(&buf->list);
 	spin_unlock(&fwc->lock);
 
-
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-	if (buf->fmt == PAGE_BUF) {
+	if (buf->is_paged_buf) {
 		int i;
 		vunmap(buf->data);
 		for (i = 0; i < buf->nr_pages; i++)
@@ -378,7 +350,89 @@ static void firmware_free_data(const struct firmware *fw)
 	fw_free_buf(fw->priv);
 }
 
+/* store the pages buffer info firmware from buf */
+static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
+{
+	fw->priv = buf;
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+	fw->pages = buf->pages;
+#endif
+	fw->size = buf->size;
+	fw->data = buf->data;
+
+	pr_debug("%s: fw-%s buf=%p data=%p size=%u\n",
+		 __func__, buf->fw_id, buf, buf->data,
+		 (unsigned int)buf->size);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void fw_name_devm_release(struct device *dev, void *res)
+{
+	struct fw_name_devm *fwn = res;
+
+	if (fwn->magic == (unsigned long)&fw_cache)
+		pr_debug("%s: fw_name-%s devm-%p released\n",
+				__func__, fwn->name, res);
+}
+
+static int fw_devm_match(struct device *dev, void *res,
+		void *match_data)
+{
+	struct fw_name_devm *fwn = res;
+
+	return (fwn->magic == (unsigned long)&fw_cache) &&
+		!strcmp(fwn->name, match_data);
+}
+
+static struct fw_name_devm *fw_find_devm_name(struct device *dev,
+		const char *name)
+{
+	struct fw_name_devm *fwn;
+
+	fwn = devres_find(dev, fw_name_devm_release,
+			  fw_devm_match, (void *)name);
+	return fwn;
+}
+
+/* add firmware name into devres list */
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+	struct fw_name_devm *fwn;
+
+	fwn = fw_find_devm_name(dev, name);
+	if (fwn)
+		return 1;
+
+	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm) +
+			   strlen(name) + 1, GFP_KERNEL);
+	if (!fwn)
+		return -ENOMEM;
+
+	fwn->magic = (unsigned long)&fw_cache;
+	strcpy(fwn->name, name);
+	devres_add(dev, fwn);
+
+	return 0;
+}
+#else
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+	return 0;
+}
+#endif
+
+
+/*
+ * user-mode helper code
+ */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+struct firmware_priv {
+	struct delayed_work timeout_work;
+	bool nowait;
+	struct device dev;
+	struct firmware_buf *buf;
+	struct firmware *fw;
+};
 
 static struct firmware_priv *to_firmware_priv(struct device *dev)
 {
@@ -393,6 +447,13 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	complete_all(&buf->completion);
 }
 
+static int loading_timeout = 60;	/* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+}
+
 static ssize_t firmware_timeout_show(struct class *class,
 				     struct class_attribute *attr,
 				     char *buf)
@@ -477,7 +538,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 /* one pages buffer should be mapped/unmapped only once */
 static int fw_map_pages_buf(struct firmware_buf *buf)
 {
-	if (buf->fmt != PAGE_BUF)
+	if (!buf->is_paged_buf)
 		return 0;
 
 	if (buf->data)
@@ -749,78 +810,112 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 exit:
 	return fw_priv;
 }
-#endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* store the pages buffer info firmware from buf */
-static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
+/* load a firmware via user helper */
+static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
+				  long timeout)
 {
-	fw->priv = buf;
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-	fw->pages = buf->pages;
-#endif
-	fw->size = buf->size;
-	fw->data = buf->data;
+	int retval = 0;
+	struct device *f_dev = &fw_priv->dev;
+	struct firmware_buf *buf = fw_priv->buf;
 
-	pr_debug("%s: fw-%s buf=%p data=%p size=%u\n",
-		 __func__, buf->fw_id, buf, buf->data,
-		 (unsigned int)buf->size);
-}
+	/* fall back on userspace loading */
+	buf->is_paged_buf = true;
 
-#ifdef CONFIG_PM_SLEEP
-static void fw_name_devm_release(struct device *dev, void *res)
-{
-	struct fw_name_devm *fwn = res;
+	dev_set_uevent_suppress(f_dev, true);
 
-	if (fwn->magic == (unsigned long)&fw_cache)
-		pr_debug("%s: fw_name-%s devm-%p released\n",
-				__func__, fwn->name, res);
-}
+	/* Need to pin this module until class device is destroyed */
+	__module_get(THIS_MODULE);
 
-static int fw_devm_match(struct device *dev, void *res,
-		void *match_data)
-{
-	struct fw_name_devm *fwn = res;
+	retval = device_add(f_dev);
+	if (retval) {
+		dev_err(f_dev, "%s: device_register failed\n", __func__);
+		goto err_put_dev;
+	}
 
-	return (fwn->magic == (unsigned long)&fw_cache) &&
-		!strcmp(fwn->name, match_data);
-}
+	retval = device_create_bin_file(f_dev, &firmware_attr_data);
+	if (retval) {
+		dev_err(f_dev, "%s: sysfs_create_bin_file failed\n", __func__);
+		goto err_del_dev;
+	}
 
-static struct fw_name_devm *fw_find_devm_name(struct device *dev,
-		const char *name)
-{
-	struct fw_name_devm *fwn;
+	retval = device_create_file(f_dev, &dev_attr_loading);
+	if (retval) {
+		dev_err(f_dev, "%s: device_create_file failed\n", __func__);
+		goto err_del_bin_attr;
+	}
 
-	fwn = devres_find(dev, fw_name_devm_release,
-			  fw_devm_match, (void *)name);
-	return fwn;
+	if (uevent) {
+		dev_set_uevent_suppress(f_dev, false);
+		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
+		if (timeout != MAX_SCHEDULE_TIMEOUT)
+			schedule_delayed_work(&fw_priv->timeout_work, timeout);
+
+		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+	}
+
+	wait_for_completion(&buf->completion);
+
+	cancel_delayed_work_sync(&fw_priv->timeout_work);
+
+	fw_priv->buf = NULL;
+
+	device_remove_file(f_dev, &dev_attr_loading);
+err_del_bin_attr:
+	device_remove_bin_file(f_dev, &firmware_attr_data);
+err_del_dev:
+	device_del(f_dev);
+err_put_dev:
+	put_device(f_dev);
+	return retval;
 }
 
-/* add firmware name into devres list */
-static int fw_add_devm_name(struct device *dev, const char *name)
+static int fw_load_from_user_helper(struct firmware *firmware,
+				    const char *name, struct device *device,
+				    bool uevent, bool nowait)
 {
-	struct fw_name_devm *fwn;
+	struct firmware_priv *fw_priv;
+	long timeout;
+	int ret;
 
-	fwn = fw_find_devm_name(dev, name);
-	if (fwn)
-		return 1;
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv))
+		return PTR_ERR(fw_priv);
 
-	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm) +
-			   strlen(name) + 1, GFP_KERNEL);
-	if (!fwn)
-		return -ENOMEM;
+	fw_priv->buf = firmware->priv;
 
-	fwn->magic = (unsigned long)&fw_cache;
-	strcpy(fwn->name, name);
-	devres_add(dev, fwn);
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			kfree(fw_priv);
+			return -EAGAIN;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			kfree(fw_priv);
+			return ret;
+		}
+	}
 
-	return 0;
+	ret = _request_firmware_load(fw_priv, uevent, timeout);
+	usermodehelper_read_unlock();
+	return ret;
 }
-#else
-static int fw_add_devm_name(struct device *dev, const char *name)
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+static inline int
+fw_load_from_user_helper(struct firmware *firmware, const char *name,
+			 struct device *device, bool uevent, bool nowait)
 {
-	return 0;
+	return -ENOENT;
 }
-#endif
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
+
 
 /* wait until the shared firmware_buf becomes ready (or error) */
 static int sync_cached_firmware_buf(struct firmware_buf *buf)
@@ -921,112 +1016,6 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
 	return 0;
 }
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-/* load a firmware via user helper */
-static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
-				  long timeout)
-{
-	int retval = 0;
-	struct device *f_dev = &fw_priv->dev;
-	struct firmware_buf *buf = fw_priv->buf;
-
-	/* fall back on userspace loading */
-	buf->fmt = PAGE_BUF;
-
-	dev_set_uevent_suppress(f_dev, true);
-
-	/* Need to pin this module until class device is destroyed */
-	__module_get(THIS_MODULE);
-
-	retval = device_add(f_dev);
-	if (retval) {
-		dev_err(f_dev, "%s: device_register failed\n", __func__);
-		goto err_put_dev;
-	}
-
-	retval = device_create_bin_file(f_dev, &firmware_attr_data);
-	if (retval) {
-		dev_err(f_dev, "%s: sysfs_create_bin_file failed\n", __func__);
-		goto err_del_dev;
-	}
-
-	retval = device_create_file(f_dev, &dev_attr_loading);
-	if (retval) {
-		dev_err(f_dev, "%s: device_create_file failed\n", __func__);
-		goto err_del_bin_attr;
-	}
-
-	if (uevent) {
-		dev_set_uevent_suppress(f_dev, false);
-		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
-		if (timeout != MAX_SCHEDULE_TIMEOUT)
-			schedule_delayed_work(&fw_priv->timeout_work, timeout);
-
-		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
-	}
-
-	wait_for_completion(&buf->completion);
-
-	cancel_delayed_work_sync(&fw_priv->timeout_work);
-
-	fw_priv->buf = NULL;
-
-	device_remove_file(f_dev, &dev_attr_loading);
-err_del_bin_attr:
-	device_remove_bin_file(f_dev, &firmware_attr_data);
-err_del_dev:
-	device_del(f_dev);
-err_put_dev:
-	put_device(f_dev);
-	return retval;
-}
-
-static int fw_load_from_user_helper(struct firmware *firmware,
-				    const char *name, struct device *device,
-				    bool uevent, bool nowait)
-{
-	struct firmware_priv *fw_priv;
-	long timeout;
-	int ret;
-
-	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
-	if (IS_ERR(fw_priv))
-		return PTR_ERR(fw_priv);
-
-	fw_priv->buf = firmware->priv;
-
-	timeout = firmware_loading_timeout();
-	if (nowait) {
-		timeout = usermodehelper_read_lock_wait(timeout);
-		if (!timeout) {
-			dev_dbg(device, "firmware: %s loading timed out\n",
-				name);
-			kfree(fw_priv);
-			return -EAGAIN;
-		}
-	} else {
-		ret = usermodehelper_read_trylock();
-		if (WARN_ON(ret)) {
-			dev_err(device, "firmware: %s will not be loaded\n",
-				name);
-			kfree(fw_priv);
-			return ret;
-		}
-	}
-
-	ret = _request_firmware_load(fw_priv, uevent, timeout);
-	usermodehelper_read_unlock();
-	return ret;
-}
-#else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int
-fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, bool uevent, bool nowait)
-{
-	return -ENOENT;
-}
-#endif /* CONFIG_FW_LOADER_USER_HELPER */
-
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-- 
1.8.1.1

[PATCH 4/4] firmware: Ignore abort check when no user-helper is used

[Takashi Iwai]

FW_STATUS_ABORT can be set only during the user-helper invocation,
thus we can ignore the check when CONFIG_HW_LOADER_USER_HELPER is
disabled.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8c40257..9753a78 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -447,6 +447,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	complete_all(&buf->completion);
 }
 
+#define is_fw_load_aborted(buf)	\
+	test_bit(FW_STATUS_ABORT, &(buf)->status)
+
 static int loading_timeout = 60;	/* In seconds */
 
 static inline long firmware_loading_timeout(void)
@@ -914,6 +917,10 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 {
 	return -ENOENT;
 }
+
+/* No abort during direct loading */
+#define is_fw_load_aborted(buf) false
+
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 
@@ -924,7 +931,7 @@ static int sync_cached_firmware_buf(struct firmware_buf *buf)
 
 	mutex_lock(&fw_lock);
 	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (test_bit(FW_STATUS_ABORT, &buf->status)) {
+		if (is_fw_load_aborted(buf)) {
 			ret = -ENOENT;
 			break;
 		}
@@ -986,7 +993,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device)
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
+	if (!buf->size || is_fw_load_aborted(buf)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
1.8.1.1

[PATCH 0/4] firmware: Make user-mode helper optional (v5)

[Takashi Iwai]

Hi,

this is a series of revised patches for making the user-mode helper
call in the firmware loader optional.  Hopefully all things settled
down now.

v4->v5: fix invalid call of usermodehelper_unlock()


thanks,

Takashi

[PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

Since 3.7 kernel, the firmware loader can read the firmware files
directly, and the traditional user-mode helper is invoked only as a
fallback.  This seems working pretty well, and the next step would be
to reduce the redundant user-mode helper stuff in future.

This patch is a preparation for that: refactor the code for splitting
user-mode helper stuff more easily.  No functional change.

Acked-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 287 ++++++++++++++++++++++--------------------
 1 file changed, 154 insertions(+), 133 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b392b35..40ec60a 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
 	return true;
 }
 
-static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
+static bool fw_get_filesystem_firmware(struct device *device,
+				       struct firmware_buf *buf)
 {
 	int i;
 	bool success = false;
@@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
 			break;
 	}
 	__putname(path);
+
+	if (success) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+			buf->fw_id);
+		mutex_lock(&fw_lock);
+		set_bit(FW_STATUS_DONE, &buf->status);
+		complete_all(&buf->completion);
+		mutex_unlock(&fw_lock);
+	}
+
 	return success;
 }
 
@@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static void _request_firmware_cleanup(const struct firmware **firmware_p)
+/* wait until the shared firmware_buf becomes ready (or error) */
+static int sync_cached_firmware_buf(struct firmware_buf *buf)
 {
-	release_firmware(*firmware_p);
-	*firmware_p = NULL;
+	int ret = 0;
+
+	mutex_lock(&fw_lock);
+	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
+		if (test_bit(FW_STATUS_ABORT, &buf->status)) {
+			ret = -ENOENT;
+			break;
+		}
+		mutex_unlock(&fw_lock);
+		wait_for_completion(&buf->completion);
+		mutex_lock(&fw_lock);
+	}
+	mutex_unlock(&fw_lock);
+	return ret;
 }
 
-static struct firmware_priv *
-_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
-			  struct device *device, bool uevent, bool nowait)
+/* prepare firmware and firmware_buf structs;
+ * return 0 if a firmware is already assigned, 1 if need to load one,
+ * or a negative error code
+ */
+static int
+_request_firmware_prepare(struct firmware **firmware_p, const char *name,
+			  struct device *device)
 {
 	struct firmware *firmware;
-	struct firmware_priv *fw_priv = NULL;
 	struct firmware_buf *buf;
 	int ret;
 
-	if (!firmware_p)
-		return ERR_PTR(-EINVAL);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
 			__func__);
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
-		return NULL;
+		return 0; /* assigned */
 	}
 
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
-	if (!ret)
-		fw_priv = fw_create_instance(firmware, name, device,
-				uevent, nowait);
 
-	if (IS_ERR(fw_priv) || ret < 0) {
-		kfree(firmware);
-		*firmware_p = NULL;
-		return ERR_PTR(-ENOMEM);
-	} else if (fw_priv) {
-		fw_priv->buf = buf;
+	/*
+	 * bind with 'buf' now to avoid warning in failure path
+	 * of requesting firmware.
+	 */
+	firmware->priv = buf;
 
-		/*
-		 * bind with 'buf' now to avoid warning in failure path
-		 * of requesting firmware.
-		 */
-		firmware->priv = buf;
-		return fw_priv;
+	if (ret > 0) {
+		ret = sync_cached_firmware_buf(buf);
+		if (!ret) {
+			fw_set_page_data(buf, firmware);
+			return 0; /* assigned */
+		}
 	}
 
-	/* share the cached buf, which is inprogessing or completed */
- check_status:
+	if (ret < 0)
+		return ret;
+	return 1; /* need to load */
+}
+
+static int assign_firmware_buf(struct firmware *fw, struct device *device)
+{
+	struct firmware_buf *buf = fw->priv;
+
 	mutex_lock(&fw_lock);
-	if (test_bit(FW_STATUS_ABORT, &buf->status)) {
-		fw_priv = ERR_PTR(-ENOENT);
-		firmware->priv = buf;
-		_request_firmware_cleanup(firmware_p);
-		goto exit;
-	} else if (test_bit(FW_STATUS_DONE, &buf->status)) {
-		fw_priv = NULL;
-		fw_set_page_data(buf, firmware);
-		goto exit;
+	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
 	}
-	mutex_unlock(&fw_lock);
-	wait_for_completion(&buf->completion);
-	goto check_status;
 
-exit:
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	if (device)
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
 	mutex_unlock(&fw_lock);
-	return fw_priv;
+	return 0;
 }
 
+/* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
 	struct firmware_buf *buf = fw_priv->buf;
-	struct firmware_cache *fwc = &fw_cache;
-	int direct_load = 0;
-
-	/* try direct loading from fs first */
-	if (fw_get_filesystem_firmware(buf)) {
-		dev_dbg(f_dev->parent, "firmware: direct-loading"
-			" firmware %s\n", buf->fw_id);
-
-		mutex_lock(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		mutex_unlock(&fw_lock);
-		complete_all(&buf->completion);
-		direct_load = 1;
-		goto handle_fw;
-	}
 
 	/* fall back on userspace loading */
 	buf->fmt = PAGE_BUF;
@@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 
-handle_fw:
-	mutex_lock(&fw_lock);
-	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
-		retval = -ENOENT;
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * f_dev->parent may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	if (!retval && f_dev->parent)
-		fw_add_devm_name(f_dev->parent, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!retval && fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw_priv->fw);
-
 	fw_priv->buf = NULL;
-	mutex_unlock(&fw_lock);
-
-	if (direct_load)
-		goto err_put_dev;
 
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
@@ -972,6 +965,73 @@ err_put_dev:
 	return retval;
 }
 
+static int fw_load_from_user_helper(struct firmware *firmware,
+				    const char *name, struct device *device,
+				    bool uevent, bool nowait, long timeout)
+{
+	struct firmware_priv *fw_priv;
+
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv))
+		return PTR_ERR(fw_priv);
+
+	fw_priv->buf = firmware->priv;
+	return _request_firmware_load(fw_priv, uevent, timeout);
+}
+
+/* called from request_firmware() and request_firmware_work_func() */
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+		  struct device *device, bool uevent, bool nowait)
+{
+	struct firmware *fw;
+	long timeout;
+	int ret;
+
+	if (!firmware_p)
+		return -EINVAL;
+
+	ret = _request_firmware_prepare(&fw, name, device);
+	if (ret <= 0) /* error or already assigned */
+		goto out;
+
+	ret = 0;
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			ret = -EBUSY;
+			goto out;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			goto out;
+		}
+	}
+
+	if (!fw_get_filesystem_firmware(device, fw->priv))
+		ret = fw_load_from_user_helper(fw, name, device,
+					       uevent, nowait, timeout);
+	if (!ret)
+		ret = assign_firmware_buf(fw, device);
+
+	usermodehelper_read_unlock();
+
+ out:
+	if (ret < 0) {
+		release_firmware(fw);
+		fw = NULL;
+	}
+
+	*firmware_p = fw;
+	return ret;
+}
+
 /**
  * request_firmware: - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
@@ -996,26 +1056,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	struct firmware_priv *fw_priv;
-	int ret;
-
-	fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
-					    false);
-	if (IS_ERR_OR_NULL(fw_priv))
-		return PTR_RET(fw_priv);
-
-	ret = usermodehelper_read_trylock();
-	if (WARN_ON(ret)) {
-		dev_err(device, "firmware: %s will not be loaded\n", name);
-	} else {
-		ret = _request_firmware_load(fw_priv, true,
-					firmware_loading_timeout());
-		usermodehelper_read_unlock();
-	}
-	if (ret)
-		_request_firmware_cleanup(firmware_p);
-
-	return ret;
+	return _request_firmware(firmware_p, name, device, true, false);
 }
 
 /**
@@ -1046,33 +1087,13 @@ static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
-	struct firmware_priv *fw_priv;
-	long timeout;
-	int ret;
 
 	fw_work = container_of(work, struct firmware_work, work);
-	fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
-			fw_work->uevent, true);
-	if (IS_ERR_OR_NULL(fw_priv)) {
-		ret = PTR_RET(fw_priv);
-		goto out;
-	}
 
-	timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
-	if (timeout) {
-		ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
-		usermodehelper_read_unlock();
-	} else {
-		dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
-			fw_work->name);
-		ret = -EAGAIN;
-	}
-	if (ret)
-		_request_firmware_cleanup(&fw);
-
- out:
+	_request_firmware(&fw, fw_work->name, fw_work->device,
+			  fw_work->uevent, true);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device);
+	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
 	module_put(fw_work->module);
 	kfree(fw_work);
-- 
1.8.1.1

[PATCH 0/4] firmware: Make user-mode helper optional (v4)

[Takashi Iwai]

Hi Ming,

this is again revised patch series.
The usermodehelper lock is applied again for the direct loading case,
the wrongly removed put_device() is revived again (with a comment).

I have a patch on top of this for moving usermodehelper lock out of
the direct loading case, but let's discuss about a better way.


thanks,

Takashi

[PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

Since 3.7 kernel, the firmware loader can read the firmware files
directly, and the traditional user-mode helper is invoked only as a
fallback.  This seems working pretty well, and the next step would be
to reduce the redundant user-mode helper stuff in future.

This patch is a preparation for that: refactor the code for splitting
user-mode helper stuff more easily.  No functional change.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 288 +++++++++++++++++++++++-------------------
 1 file changed, 155 insertions(+), 133 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b392b35..4f4b946 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
 	return true;
 }
 
-static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
+static bool fw_get_filesystem_firmware(struct device *device,
+				       struct firmware_buf *buf)
 {
 	int i;
 	bool success = false;
@@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
 			break;
 	}
 	__putname(path);
+
+	if (success) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+			buf->fw_id);
+		mutex_lock(&fw_lock);
+		set_bit(FW_STATUS_DONE, &buf->status);
+		complete_all(&buf->completion);
+		mutex_unlock(&fw_lock);
+	}
+
 	return success;
 }
 
@@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static void _request_firmware_cleanup(const struct firmware **firmware_p)
+/* wait until the shared firmware_buf becomes ready (or error) */
+static int sync_cached_firmware_buf(struct firmware_buf *buf)
 {
-	release_firmware(*firmware_p);
-	*firmware_p = NULL;
+	int ret = 0;
+
+	mutex_lock(&fw_lock);
+	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
+		if (test_bit(FW_STATUS_ABORT, &buf->status)) {
+			ret = -ENOENT;
+			break;
+		}
+		mutex_unlock(&fw_lock);
+		wait_for_completion(&buf->completion);
+		mutex_lock(&fw_lock);
+	}
+	mutex_unlock(&fw_lock);
+	return ret;
 }
 
-static struct firmware_priv *
-_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
-			  struct device *device, bool uevent, bool nowait)
+/* prepare firmware and firmware_buf structs;
+ * return 0 if a firmware is already assigned, 1 if need to load one,
+ * or a negative error code
+ */
+static int
+_request_firmware_prepare(struct firmware **firmware_p, const char *name,
+			  struct device *device)
 {
 	struct firmware *firmware;
-	struct firmware_priv *fw_priv = NULL;
 	struct firmware_buf *buf;
 	int ret;
 
-	if (!firmware_p)
-		return ERR_PTR(-EINVAL);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
 			__func__);
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
-		return NULL;
+		return 0; /* assigned */
 	}
 
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
-	if (!ret)
-		fw_priv = fw_create_instance(firmware, name, device,
-				uevent, nowait);
 
-	if (IS_ERR(fw_priv) || ret < 0) {
-		kfree(firmware);
-		*firmware_p = NULL;
-		return ERR_PTR(-ENOMEM);
-	} else if (fw_priv) {
-		fw_priv->buf = buf;
+	/*
+	 * bind with 'buf' now to avoid warning in failure path
+	 * of requesting firmware.
+	 */
+	firmware->priv = buf;
 
-		/*
-		 * bind with 'buf' now to avoid warning in failure path
-		 * of requesting firmware.
-		 */
-		firmware->priv = buf;
-		return fw_priv;
+	if (ret > 0) {
+		ret = sync_cached_firmware_buf(buf);
+		if (!ret) {
+			fw_set_page_data(buf, firmware);
+			return 0; /* assigned */
+		}
 	}
 
-	/* share the cached buf, which is inprogessing or completed */
- check_status:
+	if (ret < 0)
+		return ret;
+	return 1; /* need to load */
+}
+
+static int assign_firmware_buf(struct firmware *fw, struct device *device)
+{
+	struct firmware_buf *buf = fw->priv;
+
 	mutex_lock(&fw_lock);
-	if (test_bit(FW_STATUS_ABORT, &buf->status)) {
-		fw_priv = ERR_PTR(-ENOENT);
-		firmware->priv = buf;
-		_request_firmware_cleanup(firmware_p);
-		goto exit;
-	} else if (test_bit(FW_STATUS_DONE, &buf->status)) {
-		fw_priv = NULL;
-		fw_set_page_data(buf, firmware);
-		goto exit;
+	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
 	}
-	mutex_unlock(&fw_lock);
-	wait_for_completion(&buf->completion);
-	goto check_status;
 
-exit:
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	if (device)
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
 	mutex_unlock(&fw_lock);
-	return fw_priv;
+	return 0;
 }
 
+/* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
 	struct firmware_buf *buf = fw_priv->buf;
-	struct firmware_cache *fwc = &fw_cache;
-	int direct_load = 0;
-
-	/* try direct loading from fs first */
-	if (fw_get_filesystem_firmware(buf)) {
-		dev_dbg(f_dev->parent, "firmware: direct-loading"
-			" firmware %s\n", buf->fw_id);
-
-		mutex_lock(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		mutex_unlock(&fw_lock);
-		complete_all(&buf->completion);
-		direct_load = 1;
-		goto handle_fw;
-	}
 
 	/* fall back on userspace loading */
 	buf->fmt = PAGE_BUF;
@@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 
-handle_fw:
-	mutex_lock(&fw_lock);
-	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
-		retval = -ENOENT;
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * f_dev->parent may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	if (!retval && f_dev->parent)
-		fw_add_devm_name(f_dev->parent, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!retval && fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw_priv->fw);
-
 	fw_priv->buf = NULL;
-	mutex_unlock(&fw_lock);
-
-	if (direct_load)
-		goto err_put_dev;
 
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
@@ -972,6 +965,74 @@ err_put_dev:
 	return retval;
 }
 
+static int fw_load_from_user_helper(struct firmware *firmware,
+				    const char *name, struct device *device,
+				    bool uevent, bool nowait, long timeout)
+{
+	struct firmware_priv *fw_priv;
+
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv))
+		return PTR_ERR(fw_priv);
+
+	fw_priv->buf = firmware->priv;
+	return _request_firmware_load(fw_priv, uevent, timeout);
+}
+
+/* called from request_firmware() and request_firmware_work_func() */
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+		  struct device *device, bool uevent, bool nowait)
+{
+	struct firmware *fw;
+	long timeout;
+	int ret;
+
+	if (!firmware_p)
+		return -EINVAL;
+
+	ret = _request_firmware_prepare(&fw, name, device);
+	if (ret <= 0) /* error or already assigned */
+		goto out;
+
+	ret = 0;
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			ret = -EBUSY;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+		}
+	}
+
+	if (!ret) {
+		if (!fw_get_filesystem_firmware(device, fw->priv))
+			ret = fw_load_from_user_helper(fw, name, device,
+						       uevent, nowait,
+						       timeout);
+		if (!ret)
+			ret = assign_firmware_buf(fw, device);
+	}
+
+	usermodehelper_read_unlock();
+
+ out:
+	if (ret < 0) {
+		release_firmware(fw);
+		fw = NULL;
+	}
+
+	*firmware_p = fw;
+	return ret;
+}
+
 /**
  * request_firmware: - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
@@ -996,26 +1057,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	struct firmware_priv *fw_priv;
-	int ret;
-
-	fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
-					    false);
-	if (IS_ERR_OR_NULL(fw_priv))
-		return PTR_RET(fw_priv);
-
-	ret = usermodehelper_read_trylock();
-	if (WARN_ON(ret)) {
-		dev_err(device, "firmware: %s will not be loaded\n", name);
-	} else {
-		ret = _request_firmware_load(fw_priv, true,
-					firmware_loading_timeout());
-		usermodehelper_read_unlock();
-	}
-	if (ret)
-		_request_firmware_cleanup(firmware_p);
-
-	return ret;
+	return _request_firmware(firmware_p, name, device, true, false);
 }
 
 /**
@@ -1046,33 +1088,13 @@ static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
-	struct firmware_priv *fw_priv;
-	long timeout;
-	int ret;
 
 	fw_work = container_of(work, struct firmware_work, work);
-	fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
-			fw_work->uevent, true);
-	if (IS_ERR_OR_NULL(fw_priv)) {
-		ret = PTR_RET(fw_priv);
-		goto out;
-	}
 
-	timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
-	if (timeout) {
-		ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
-		usermodehelper_read_unlock();
-	} else {
-		dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
-			fw_work->name);
-		ret = -EAGAIN;
-	}
-	if (ret)
-		_request_firmware_cleanup(&fw);
-
- out:
+	_request_firmware(&fw, fw_work->name, fw_work->device,
+			  fw_work->uevent, true);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device);
+	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
 	module_put(fw_work->module);
 	kfree(fw_work);
-- 
1.8.1.1

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

On Wed, Jan 30, 2013 at 6:35 PM, Takashi Iwai <tiwai@suse.de> wrote:
> Since 3.7 kernel, the firmware loader can read the firmware files
> directly, and the traditional user-mode helper is invoked only as a
> fallback.  This seems working pretty well, and the next step would be
> to reduce the redundant user-mode helper stuff in future.
>
> This patch is a preparation for that: refactor the code for splitting
> user-mode helper stuff more easily.  No functional change.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 288 +++++++++++++++++++++++-------------------
>  1 file changed, 155 insertions(+), 133 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b392b35..4f4b946 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>         return true;
>  }
>
> -static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> +static bool fw_get_filesystem_firmware(struct device *device,
> +                                      struct firmware_buf *buf)
>  {
>         int i;
>         bool success = false;
> @@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
>                         break;
>         }
>         __putname(path);
> +
> +       if (success) {
> +               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> +                       buf->fw_id);
> +               mutex_lock(&fw_lock);
> +               set_bit(FW_STATUS_DONE, &buf->status);
> +               complete_all(&buf->completion);
> +               mutex_unlock(&fw_lock);
> +       }
> +
>         return success;
>  }
>
> @@ -796,99 +807,112 @@ static int fw_add_devm_name(struct device *dev, const char *name)
>  }
>  #endif
>
> -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> +/* wait until the shared firmware_buf becomes ready (or error) */
> +static int sync_cached_firmware_buf(struct firmware_buf *buf)
>  {
> -       release_firmware(*firmware_p);
> -       *firmware_p = NULL;
> +       int ret = 0;
> +
> +       mutex_lock(&fw_lock);
> +       while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> +               if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> +                       ret = -ENOENT;
> +                       break;
> +               }
> +               mutex_unlock(&fw_lock);
> +               wait_for_completion(&buf->completion);
> +               mutex_lock(&fw_lock);
> +       }
> +       mutex_unlock(&fw_lock);
> +       return ret;
>  }
>
> -static struct firmware_priv *
> -_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
> -                         struct device *device, bool uevent, bool nowait)
> +/* prepare firmware and firmware_buf structs;
> + * return 0 if a firmware is already assigned, 1 if need to load one,
> + * or a negative error code
> + */
> +static int
> +_request_firmware_prepare(struct firmware **firmware_p, const char *name,
> +                         struct device *device)
>  {
>         struct firmware *firmware;
> -       struct firmware_priv *fw_priv = NULL;
>         struct firmware_buf *buf;
>         int ret;
>
> -       if (!firmware_p)
> -               return ERR_PTR(-EINVAL);
> -
>         *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
>         if (!firmware) {
>                 dev_err(device, "%s: kmalloc(struct firmware) failed\n",
>                         __func__);
> -               return ERR_PTR(-ENOMEM);
> +               return -ENOMEM;
>         }
>
>         if (fw_get_builtin_firmware(firmware, name)) {
>                 dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> -               return NULL;
> +               return 0; /* assigned */
>         }
>
>         ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
> -       if (!ret)
> -               fw_priv = fw_create_instance(firmware, name, device,
> -                               uevent, nowait);
>
> -       if (IS_ERR(fw_priv) || ret < 0) {
> -               kfree(firmware);
> -               *firmware_p = NULL;
> -               return ERR_PTR(-ENOMEM);
> -       } else if (fw_priv) {
> -               fw_priv->buf = buf;
> +       /*
> +        * bind with 'buf' now to avoid warning in failure path
> +        * of requesting firmware.
> +        */
> +       firmware->priv = buf;
>
> -               /*
> -                * bind with 'buf' now to avoid warning in failure path
> -                * of requesting firmware.
> -                */
> -               firmware->priv = buf;
> -               return fw_priv;
> +       if (ret > 0) {
> +               ret = sync_cached_firmware_buf(buf);
> +               if (!ret) {
> +                       fw_set_page_data(buf, firmware);
> +                       return 0; /* assigned */
> +               }
>         }
>
> -       /* share the cached buf, which is inprogessing or completed */
> - check_status:
> +       if (ret < 0)
> +               return ret;
> +       return 1; /* need to load */
> +}
> +
> +static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +{
> +       struct firmware_buf *buf = fw->priv;
> +
>         mutex_lock(&fw_lock);
> -       if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> -               fw_priv = ERR_PTR(-ENOENT);
> -               firmware->priv = buf;
> -               _request_firmware_cleanup(firmware_p);
> -               goto exit;
> -       } else if (test_bit(FW_STATUS_DONE, &buf->status)) {
> -               fw_priv = NULL;
> -               fw_set_page_data(buf, firmware);
> -               goto exit;
> +       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
> +               mutex_unlock(&fw_lock);
> +               return -ENOENT;
>         }
> -       mutex_unlock(&fw_lock);
> -       wait_for_completion(&buf->completion);
> -       goto check_status;
>
> -exit:
> +       /*
> +        * add firmware name into devres list so that we can auto cache
> +        * and uncache firmware for device.
> +        *
> +        * device may has been deleted already, but the problem
> +        * should be fixed in devres or driver core.
> +        */
> +       if (device)
> +               fw_add_devm_name(device, buf->fw_id);
> +
> +       /*
> +        * After caching firmware image is started, let it piggyback
> +        * on request firmware.
> +        */
> +       if (buf->fwc->state == FW_LOADER_START_CACHE) {
> +               if (fw_cache_piggyback_on_request(buf->fw_id))
> +                       kref_get(&buf->ref);
> +       }
> +
> +       /* pass the pages buffer to driver at the last minute */
> +       fw_set_page_data(buf, fw);
>         mutex_unlock(&fw_lock);
> -       return fw_priv;
> +       return 0;
>  }
>
> +/* load a firmware via user helper */
>  static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                                   long timeout)
>  {
>         int retval = 0;
>         struct device *f_dev = &fw_priv->dev;
>         struct firmware_buf *buf = fw_priv->buf;
> -       struct firmware_cache *fwc = &fw_cache;
> -       int direct_load = 0;
> -
> -       /* try direct loading from fs first */
> -       if (fw_get_filesystem_firmware(buf)) {
> -               dev_dbg(f_dev->parent, "firmware: direct-loading"
> -                       " firmware %s\n", buf->fw_id);
> -
> -               mutex_lock(&fw_lock);
> -               set_bit(FW_STATUS_DONE, &buf->status);
> -               mutex_unlock(&fw_lock);
> -               complete_all(&buf->completion);
> -               direct_load = 1;
> -               goto handle_fw;
> -       }
>
>         /* fall back on userspace loading */
>         buf->fmt = PAGE_BUF;
> @@ -929,38 +953,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>
>         cancel_delayed_work_sync(&fw_priv->timeout_work);
>
> -handle_fw:
> -       mutex_lock(&fw_lock);
> -       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> -               retval = -ENOENT;
> -
> -       /*
> -        * add firmware name into devres list so that we can auto cache
> -        * and uncache firmware for device.
> -        *
> -        * f_dev->parent may has been deleted already, but the problem
> -        * should be fixed in devres or driver core.
> -        */
> -       if (!retval && f_dev->parent)
> -               fw_add_devm_name(f_dev->parent, buf->fw_id);
> -
> -       /*
> -        * After caching firmware image is started, let it piggyback
> -        * on request firmware.
> -        */
> -       if (!retval && fwc->state == FW_LOADER_START_CACHE) {
> -               if (fw_cache_piggyback_on_request(buf->fw_id))
> -                       kref_get(&buf->ref);
> -       }
> -
> -       /* pass the pages buffer to driver at the last minute */
> -       fw_set_page_data(buf, fw_priv->fw);
> -
>         fw_priv->buf = NULL;
> -       mutex_unlock(&fw_lock);
> -
> -       if (direct_load)
> -               goto err_put_dev;
>
>         device_remove_file(f_dev, &dev_attr_loading);
>  err_del_bin_attr:
> @@ -972,6 +965,74 @@ err_put_dev:
>         return retval;
>  }
>
> +static int fw_load_from_user_helper(struct firmware *firmware,
> +                                   const char *name, struct device *device,
> +                                   bool uevent, bool nowait, long timeout)
> +{
> +       struct firmware_priv *fw_priv;
> +
> +       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> +       if (IS_ERR(fw_priv))
> +               return PTR_ERR(fw_priv);
> +
> +       fw_priv->buf = firmware->priv;
> +       return _request_firmware_load(fw_priv, uevent, timeout);
> +}
> +
> +/* called from request_firmware() and request_firmware_work_func() */
> +static int
> +_request_firmware(const struct firmware **firmware_p, const char *name,
> +                 struct device *device, bool uevent, bool nowait)
> +{
> +       struct firmware *fw;
> +       long timeout;
> +       int ret;
> +
> +       if (!firmware_p)
> +               return -EINVAL;
> +
> +       ret = _request_firmware_prepare(&fw, name, device);
> +       if (ret <= 0) /* error or already assigned */
> +               goto out;
> +
> +       ret = 0;
> +       timeout = firmware_loading_timeout();

The above line may be moved below the line of 'if (nowait) '.

> +       if (nowait) {
> +               timeout = usermodehelper_read_lock_wait(timeout);
> +               if (!timeout) {
> +                       dev_dbg(device, "firmware: %s loading timed out\n",
> +                               name);
> +                       ret = -EBUSY;
> +               }
> +       } else {
> +               ret = usermodehelper_read_trylock();
> +               if (WARN_ON(ret)) {
> +                       dev_err(device, "firmware: %s will not be loaded\n",
> +                               name);
> +               }
> +       }
> +
> +       if (!ret) {
> +               if (!fw_get_filesystem_firmware(device, fw->priv))
> +                       ret = fw_load_from_user_helper(fw, name, device,
> +                                                      uevent, nowait,
> +                                                      timeout);
> +               if (!ret)
> +                       ret = assign_firmware_buf(fw, device);
> +       }
> +
> +       usermodehelper_read_unlock();

The above should be move inside the above 'if (!ret) {...}'

Except for the above two comments, looks the patch v4 is fine.

Please CC greg-kh when you send v5, and you may add my ack
for the patchset.

            Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Thu, 31 Jan 2013 17:48:34 +0800,
Ming Lei wrote:
> 
(snip)
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       long timeout;
> > +       int ret;
> > +
> > +       if (!firmware_p)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret <= 0) /* error or already assigned */
> > +               goto out;
> > +
> > +       ret = 0;
> > +       timeout = firmware_loading_timeout();
> 
> The above line may be moved below the line of 'if (nowait) '.

I thought of that, too, but later found that the timeout is used in
the call of fw_load_from_user_helper() below.

> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       ret = -EBUSY;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +               }
> > +       }
> > +
> > +       if (!ret) {
> > +               if (!fw_get_filesystem_firmware(device, fw->priv))
> > +                       ret = fw_load_from_user_helper(fw, name, device,
> > +                                                      uevent, nowait,
> > +                                                      timeout);
> > +               if (!ret)
> > +                       ret = assign_firmware_buf(fw, device);
> > +       }
> > +
> > +       usermodehelper_read_unlock();
> 
> The above should be move inside the above 'if (!ret) {...}'

Yep, but it would make the code less understandable.
I'll make rather the error path of usermodehelper lock goto's.

> 
> Except for the above two comments, looks the patch v4 is fine.
> 
> Please CC greg-kh when you send v5, and you may add my ack
> for the patchset.
> 
>             Acked-by: Ming Lei <ming.lei@canonical.com>

OK, thanks!


Takashi

[PATCH 0/4] firmware: Make user-mode helper optional (v2)

[Takashi Iwai]

Hi Ming,

this is a series of revised patches to make the user-mode helper in the
firmware loader optional.  A few bugs (typos) have been fixed during
this rewrite, and a small optimization was added in the last patch.
I hope this would be good enough for review.


thanks,

Takashi

[PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

Since 3.7 kernel, the firmware loader can read the firmware files
directly, and the traditional user-mode helper is invoked only as a
fallback.  This seems working pretty well, and the next step would be
to reduce the redundant user-mode helper stuff in future.

This patch is a preparation for that: refactor the code for splitting
user-mode helper stuff more easily.  No functional change.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_class.c | 286 +++++++++++++++++++++++-------------------
 1 file changed, 159 insertions(+), 127 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b392b35..7da18e3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
 	return true;
 }
 
-static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
+static bool fw_get_filesystem_firmware(struct device *device,
+				       struct firmware_buf *buf)
 {
 	int i;
 	bool success = false;
@@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
 			break;
 	}
 	__putname(path);
+
+	if (success) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+			buf->fw_id);
+		mutex_lock(&fw_lock);
+		set_bit(FW_STATUS_DONE, &buf->status);
+		complete_all(&buf->completion);
+		mutex_unlock(&fw_lock);
+	}
+
 	return success;
 }
 
@@ -796,99 +807,122 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static void _request_firmware_cleanup(const struct firmware **firmware_p)
+static void _request_firmware_cleanup(struct firmware **firmware_p)
 {
 	release_firmware(*firmware_p);
 	*firmware_p = NULL;
 }
 
-static struct firmware_priv *
-_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
-			  struct device *device, bool uevent, bool nowait)
+/* wait until the shared firmware_buf becomes ready (or error) */
+static int sync_cached_firmware_buf(struct firmware_buf *buf)
+{
+	int ret = 0;
+
+	mutex_lock(&fw_lock);
+	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
+		if (test_bit(FW_STATUS_ABORT, &buf->status)) {
+			ret = -ENOENT;
+			break;
+		}
+		mutex_unlock(&fw_lock);
+		wait_for_completion(&buf->completion);
+		mutex_lock(&fw_lock);
+	}
+	mutex_unlock(&fw_lock);
+	return ret;
+}
+
+/* prepare firmware and firmware_buf structs;
+ * return 1 if a firmware is already assigned, 0 if need to load one,
+ * or a negative error code
+ */
+static int
+_request_firmware_prepare(struct firmware **firmware_p, const char *name,
+			  struct device *device)
 {
 	struct firmware *firmware;
-	struct firmware_priv *fw_priv = NULL;
 	struct firmware_buf *buf;
 	int ret;
 
 	if (!firmware_p)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
 			__func__);
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
-		return NULL;
+		return 1; /* assigned */
 	}
 
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
-	if (!ret)
-		fw_priv = fw_create_instance(firmware, name, device,
-				uevent, nowait);
 
-	if (IS_ERR(fw_priv) || ret < 0) {
-		kfree(firmware);
-		*firmware_p = NULL;
-		return ERR_PTR(-ENOMEM);
-	} else if (fw_priv) {
-		fw_priv->buf = buf;
+	/*
+	 * bind with 'buf' now to avoid warning in failure path
+	 * of requesting firmware.
+	 */
+	firmware->priv = buf;
 
-		/*
-		 * bind with 'buf' now to avoid warning in failure path
-		 * of requesting firmware.
-		 */
-		firmware->priv = buf;
-		return fw_priv;
+	if (ret > 0) {
+		ret = sync_cached_firmware_buf(buf);
+		if (!ret) {
+			fw_set_page_data(buf, firmware);
+			return 1; /* assigned */
+		}
 	}
 
-	/* share the cached buf, which is inprogessing or completed */
- check_status:
-	mutex_lock(&fw_lock);
-	if (test_bit(FW_STATUS_ABORT, &buf->status)) {
-		fw_priv = ERR_PTR(-ENOENT);
-		firmware->priv = buf;
+	if (ret < 0)
 		_request_firmware_cleanup(firmware_p);
-		goto exit;
-	} else if (test_bit(FW_STATUS_DONE, &buf->status)) {
-		fw_priv = NULL;
-		fw_set_page_data(buf, firmware);
-		goto exit;
+
+	return ret;
+}
+
+static int assign_firmware_buf(struct firmware *fw, struct device *device)
+{
+	struct firmware_buf *buf = fw->priv;
+
+	mutex_lock(&fw_lock);
+	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
 	}
-	mutex_unlock(&fw_lock);
-	wait_for_completion(&buf->completion);
-	goto check_status;
 
-exit:
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	if (device)
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
 	mutex_unlock(&fw_lock);
-	return fw_priv;
+	return 0;
 }
 
+/* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
 	struct firmware_buf *buf = fw_priv->buf;
-	struct firmware_cache *fwc = &fw_cache;
-	int direct_load = 0;
-
-	/* try direct loading from fs first */
-	if (fw_get_filesystem_firmware(buf)) {
-		dev_dbg(f_dev->parent, "firmware: direct-loading"
-			" firmware %s\n", buf->fw_id);
-
-		mutex_lock(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		mutex_unlock(&fw_lock);
-		complete_all(&buf->completion);
-		direct_load = 1;
-		goto handle_fw;
-	}
 
 	/* fall back on userspace loading */
 	buf->fmt = PAGE_BUF;
@@ -929,38 +963,10 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 
-handle_fw:
-	mutex_lock(&fw_lock);
-	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
-		retval = -ENOENT;
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * f_dev->parent may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	if (!retval && f_dev->parent)
-		fw_add_devm_name(f_dev->parent, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!retval && fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw_priv->fw);
+	if (!retval)
+		retval = assign_firmware_buf(fw_priv->fw, f_dev->parent);
 
 	fw_priv->buf = NULL;
-	mutex_unlock(&fw_lock);
-
-	if (direct_load)
-		goto err_put_dev;
 
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
@@ -972,6 +978,71 @@ err_put_dev:
 	return retval;
 }
 
+static int fw_load_from_user_helper(struct firmware *firmware,
+				    const char *name, struct device *device,
+				    bool uevent, bool nowait)
+{
+	struct firmware_priv *fw_priv;
+	long timeout;
+	int ret;
+
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv))
+		return PTR_ERR(fw_priv);
+
+	fw_priv->buf = firmware->priv;
+
+	timeout = firmware_loading_timeout();
+	if (nowait) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			kfree(fw_priv);
+			return -EAGAIN;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			kfree(fw_priv);
+			return ret;
+		}
+	}
+
+	ret = _request_firmware_load(fw_priv, uevent, timeout);
+	usermodehelper_read_unlock();
+	return ret;
+}
+
+/* called from request_firmware() and request_firmware_work_func() */
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+		  struct device *device, bool uevent, bool nowait)
+{
+	struct firmware *fw;
+	int ret;
+
+	ret = _request_firmware_prepare(&fw, name, device);
+	if (ret < 0)
+		return ret;
+	else if (ret) { /* assigned from either built-in or cache */
+		*firmware_p = fw;
+		return 0;
+	}
+
+	if (fw_get_filesystem_firmware(device, fw->priv))
+		ret = assign_firmware_buf(fw, device);
+	else
+		ret = fw_load_from_user_helper(fw, name, device,
+					       uevent, nowait);
+	if (ret)
+		_request_firmware_cleanup(&fw);
+	*firmware_p = fw;
+	return ret;
+}
+
 /**
  * request_firmware: - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
@@ -996,26 +1067,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	struct firmware_priv *fw_priv;
-	int ret;
-
-	fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
-					    false);
-	if (IS_ERR_OR_NULL(fw_priv))
-		return PTR_RET(fw_priv);
-
-	ret = usermodehelper_read_trylock();
-	if (WARN_ON(ret)) {
-		dev_err(device, "firmware: %s will not be loaded\n", name);
-	} else {
-		ret = _request_firmware_load(fw_priv, true,
-					firmware_loading_timeout());
-		usermodehelper_read_unlock();
-	}
-	if (ret)
-		_request_firmware_cleanup(firmware_p);
-
-	return ret;
+	return _request_firmware(firmware_p, name, device, true, false);
 }
 
 /**
@@ -1046,31 +1098,11 @@ static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
-	struct firmware_priv *fw_priv;
-	long timeout;
-	int ret;
 
 	fw_work = container_of(work, struct firmware_work, work);
-	fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
-			fw_work->uevent, true);
-	if (IS_ERR_OR_NULL(fw_priv)) {
-		ret = PTR_RET(fw_priv);
-		goto out;
-	}
-
-	timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
-	if (timeout) {
-		ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
-		usermodehelper_read_unlock();
-	} else {
-		dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
-			fw_work->name);
-		ret = -EAGAIN;
-	}
-	if (ret)
-		_request_firmware_cleanup(&fw);
 
- out:
+	_request_firmware(&fw, fw_work->name, fw_work->device,
+			  fw_work->uevent, true);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device);
 
-- 
1.8.1.1

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Ming Lei]

Hi Takashi,

On Sat, Jan 26, 2013 at 12:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Since 3.7 kernel, the firmware loader can read the firmware files
> directly, and the traditional user-mode helper is invoked only as a
> fallback.  This seems working pretty well, and the next step would be
> to reduce the redundant user-mode helper stuff in future.
>
> This patch is a preparation for that: refactor the code for splitting
> user-mode helper stuff more easily.  No functional change.

Looks a good code refactoring, :-)

>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/firmware_class.c | 286 +++++++++++++++++++++++-------------------
>  1 file changed, 159 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b392b35..7da18e3 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>         return true;
>  }
>
> -static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> +static bool fw_get_filesystem_firmware(struct device *device,
> +                                      struct firmware_buf *buf)
>  {
>         int i;
>         bool success = false;
> @@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
>                         break;
>         }
>         __putname(path);
> +
> +       if (success) {
> +               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> +                       buf->fw_id);
> +               mutex_lock(&fw_lock);
> +               set_bit(FW_STATUS_DONE, &buf->status);
> +               complete_all(&buf->completion);
> +               mutex_unlock(&fw_lock);
> +       }
> +
>         return success;
>  }
>
> @@ -796,99 +807,122 @@ static int fw_add_devm_name(struct device *dev, const char *name)
>  }
>  #endif
>
> -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> +static void _request_firmware_cleanup(struct firmware **firmware_p)
>  {
>         release_firmware(*firmware_p);
>         *firmware_p = NULL;
>  }
>
> -static struct firmware_priv *
> -_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
> -                         struct device *device, bool uevent, bool nowait)
> +/* wait until the shared firmware_buf becomes ready (or error) */
> +static int sync_cached_firmware_buf(struct firmware_buf *buf)
> +{
> +       int ret = 0;
> +
> +       mutex_lock(&fw_lock);
> +       while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> +               if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> +                       ret = -ENOENT;
> +                       break;
> +               }
> +               mutex_unlock(&fw_lock);
> +               wait_for_completion(&buf->completion);
> +               mutex_lock(&fw_lock);
> +       }
> +       mutex_unlock(&fw_lock);
> +       return ret;
> +}
> +
> +/* prepare firmware and firmware_buf structs;
> + * return 1 if a firmware is already assigned, 0 if need to load one,
> + * or a negative error code
> + */
> +static int
> +_request_firmware_prepare(struct firmware **firmware_p, const char *name,
> +                         struct device *device)
>  {
>         struct firmware *firmware;

Since the 'firmware_p' isn't marked as const, you can use it directly and
remove the above local variable.

> -       struct firmware_priv *fw_priv = NULL;
>         struct firmware_buf *buf;
>         int ret;
>
>         if (!firmware_p)

Now the check isn't needed any more here, and it should be done in the entry
of _request_firmware().

> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>
>         *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
>         if (!firmware) {
>                 dev_err(device, "%s: kmalloc(struct firmware) failed\n",
>                         __func__);
> -               return ERR_PTR(-ENOMEM);
> +               return -ENOMEM;
>         }
>
>         if (fw_get_builtin_firmware(firmware, name)) {
>                 dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> -               return NULL;
> +               return 1; /* assigned */
>         }
>
>         ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
> -       if (!ret)
> -               fw_priv = fw_create_instance(firmware, name, device,
> -                               uevent, nowait);
>
> -       if (IS_ERR(fw_priv) || ret < 0) {
> -               kfree(firmware);
> -               *firmware_p = NULL;
> -               return ERR_PTR(-ENOMEM);
> -       } else if (fw_priv) {
> -               fw_priv->buf = buf;
> +       /*
> +        * bind with 'buf' now to avoid warning in failure path
> +        * of requesting firmware.
> +        */
> +       firmware->priv = buf;
>
> -               /*
> -                * bind with 'buf' now to avoid warning in failure path
> -                * of requesting firmware.
> -                */
> -               firmware->priv = buf;
> -               return fw_priv;
> +       if (ret > 0) {
> +               ret = sync_cached_firmware_buf(buf);
> +               if (!ret) {
> +                       fw_set_page_data(buf, firmware);
> +                       return 1; /* assigned */
> +               }
>         }
>
> -       /* share the cached buf, which is inprogessing or completed */
> - check_status:
> -       mutex_lock(&fw_lock);
> -       if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> -               fw_priv = ERR_PTR(-ENOENT);
> -               firmware->priv = buf;
> +       if (ret < 0)
>                 _request_firmware_cleanup(firmware_p);

It is better to do cleanup firmware centralizedly in _request_firmware().

> -               goto exit;
> -       } else if (test_bit(FW_STATUS_DONE, &buf->status)) {
> -               fw_priv = NULL;
> -               fw_set_page_data(buf, firmware);
> -               goto exit;
> +
> +       return ret;
> +}
> +
> +static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +{
> +       struct firmware_buf *buf = fw->priv;
> +
> +       mutex_lock(&fw_lock);
> +       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
> +               mutex_unlock(&fw_lock);
> +               return -ENOENT;
>         }
> -       mutex_unlock(&fw_lock);
> -       wait_for_completion(&buf->completion);
> -       goto check_status;
>
> -exit:
> +       /*
> +        * add firmware name into devres list so that we can auto cache
> +        * and uncache firmware for device.
> +        *
> +        * device may has been deleted already, but the problem
> +        * should be fixed in devres or driver core.
> +        */
> +       if (device)
> +               fw_add_devm_name(device, buf->fw_id);
> +
> +       /*
> +        * After caching firmware image is started, let it piggyback
> +        * on request firmware.
> +        */
> +       if (buf->fwc->state == FW_LOADER_START_CACHE) {
> +               if (fw_cache_piggyback_on_request(buf->fw_id))
> +                       kref_get(&buf->ref);
> +       }
> +
> +       /* pass the pages buffer to driver at the last minute */
> +       fw_set_page_data(buf, fw);
>         mutex_unlock(&fw_lock);
> -       return fw_priv;
> +       return 0;
>  }
>
> +/* load a firmware via user helper */
>  static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                                   long timeout)
>  {
>         int retval = 0;
>         struct device *f_dev = &fw_priv->dev;
>         struct firmware_buf *buf = fw_priv->buf;
> -       struct firmware_cache *fwc = &fw_cache;
> -       int direct_load = 0;
> -
> -       /* try direct loading from fs first */
> -       if (fw_get_filesystem_firmware(buf)) {
> -               dev_dbg(f_dev->parent, "firmware: direct-loading"
> -                       " firmware %s\n", buf->fw_id);
> -
> -               mutex_lock(&fw_lock);
> -               set_bit(FW_STATUS_DONE, &buf->status);
> -               mutex_unlock(&fw_lock);
> -               complete_all(&buf->completion);
> -               direct_load = 1;
> -               goto handle_fw;
> -       }
>
>         /* fall back on userspace loading */
>         buf->fmt = PAGE_BUF;
> @@ -929,38 +963,10 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>
>         cancel_delayed_work_sync(&fw_priv->timeout_work);
>
> -handle_fw:
> -       mutex_lock(&fw_lock);
> -       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> -               retval = -ENOENT;
> -
> -       /*
> -        * add firmware name into devres list so that we can auto cache
> -        * and uncache firmware for device.
> -        *
> -        * f_dev->parent may has been deleted already, but the problem
> -        * should be fixed in devres or driver core.
> -        */
> -       if (!retval && f_dev->parent)
> -               fw_add_devm_name(f_dev->parent, buf->fw_id);
> -
> -       /*
> -        * After caching firmware image is started, let it piggyback
> -        * on request firmware.
> -        */
> -       if (!retval && fwc->state == FW_LOADER_START_CACHE) {
> -               if (fw_cache_piggyback_on_request(buf->fw_id))
> -                       kref_get(&buf->ref);
> -       }
> -
> -       /* pass the pages buffer to driver at the last minute */
> -       fw_set_page_data(buf, fw_priv->fw);
> +       if (!retval)
> +               retval = assign_firmware_buf(fw_priv->fw, f_dev->parent);
>
>         fw_priv->buf = NULL;
> -       mutex_unlock(&fw_lock);
> -
> -       if (direct_load)
> -               goto err_put_dev;
>
>         device_remove_file(f_dev, &dev_attr_loading);
>  err_del_bin_attr:
> @@ -972,6 +978,71 @@ err_put_dev:
>         return retval;
>  }
>
> +static int fw_load_from_user_helper(struct firmware *firmware,
> +                                   const char *name, struct device *device,
> +                                   bool uevent, bool nowait)
> +{
> +       struct firmware_priv *fw_priv;
> +       long timeout;
> +       int ret;
> +
> +       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> +       if (IS_ERR(fw_priv))
> +               return PTR_ERR(fw_priv);
> +
> +       fw_priv->buf = firmware->priv;
> +
> +       timeout = firmware_loading_timeout();
> +       if (nowait) {
> +               timeout = usermodehelper_read_lock_wait(timeout);
> +               if (!timeout) {
> +                       dev_dbg(device, "firmware: %s loading timed out\n",
> +                               name);
> +                       kfree(fw_priv);
> +                       return -EAGAIN;
> +               }
> +       } else {
> +               ret = usermodehelper_read_trylock();
> +               if (WARN_ON(ret)) {
> +                       dev_err(device, "firmware: %s will not be loaded\n",
> +                               name);
> +                       kfree(fw_priv);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = _request_firmware_load(fw_priv, uevent, timeout);
> +       usermodehelper_read_unlock();
> +       return ret;
> +}
> +
> +/* called from request_firmware() and request_firmware_work_func() */
> +static int
> +_request_firmware(const struct firmware **firmware_p, const char *name,
> +                 struct device *device, bool uevent, bool nowait)
> +{
> +       struct firmware *fw;
> +       int ret;
> +
> +       ret = _request_firmware_prepare(&fw, name, device);
> +       if (ret < 0)
> +               return ret;

It is better to handle the cleanup firmware outside _request_firmware_prepare,
and 'goto' is preferred.

> +       else if (ret) { /* assigned from either built-in or cache */
> +               *firmware_p = fw;
> +               return 0;

You might use 'goto' to do above.

> +       }
> +
> +       if (fw_get_filesystem_firmware(device, fw->priv))
> +               ret = assign_firmware_buf(fw, device);
> +       else
> +               ret = fw_load_from_user_helper(fw, name, device,
> +                                              uevent, nowait);

How about changing above as below and remove assign_firmware_buf()
from _request_firmware_load()?

	if (ret = !fw_get_filesystem_firmware(device, fw->priv))
		ret = fw_load_from_user_helper(fw, name, device,
						uevent, nowait);

	if (!ret)
		ret = assign_firmware_buf(fw, device);

So we can assign buffer only in one place.

> +       if (ret)
> +               _request_firmware_cleanup(&fw);
> +       *firmware_p = fw;
> +       return ret;
> +}
> +
>  /**
>   * request_firmware: - send firmware request and wait for it
>   * @firmware_p: pointer to firmware image
> @@ -996,26 +1067,7 @@ int
>  request_firmware(const struct firmware **firmware_p, const char *name,
>                   struct device *device)
>  {
> -       struct firmware_priv *fw_priv;
> -       int ret;
> -
> -       fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
> -                                           false);
> -       if (IS_ERR_OR_NULL(fw_priv))
> -               return PTR_RET(fw_priv);
> -
> -       ret = usermodehelper_read_trylock();
> -       if (WARN_ON(ret)) {
> -               dev_err(device, "firmware: %s will not be loaded\n", name);
> -       } else {
> -               ret = _request_firmware_load(fw_priv, true,
> -                                       firmware_loading_timeout());
> -               usermodehelper_read_unlock();
> -       }
> -       if (ret)
> -               _request_firmware_cleanup(firmware_p);
> -
> -       return ret;
> +       return _request_firmware(firmware_p, name, device, true, false);
>  }
>
>  /**
> @@ -1046,31 +1098,11 @@ static void request_firmware_work_func(struct work_struct *work)
>  {
>         struct firmware_work *fw_work;
>         const struct firmware *fw;
> -       struct firmware_priv *fw_priv;
> -       long timeout;
> -       int ret;
>
>         fw_work = container_of(work, struct firmware_work, work);
> -       fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
> -                       fw_work->uevent, true);
> -       if (IS_ERR_OR_NULL(fw_priv)) {
> -               ret = PTR_RET(fw_priv);
> -               goto out;
> -       }
> -
> -       timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
> -       if (timeout) {
> -               ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
> -               usermodehelper_read_unlock();
> -       } else {
> -               dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
> -                       fw_work->name);
> -               ret = -EAGAIN;
> -       }
> -       if (ret)
> -               _request_firmware_cleanup(&fw);
>
> - out:
> +       _request_firmware(&fw, fw_work->name, fw_work->device,
> +                         fw_work->uevent, true);
>         fw_work->cont(fw, fw_work->context);
>         put_device(fw_work->device);
>

Thanks,
--
Ming Lei

Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai]

At Tue, 29 Jan 2013 18:24:04 +0800,
Ming Lei wrote:
> 
> Hi Takashi,
> 
> On Sat, Jan 26, 2013 at 12:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Since 3.7 kernel, the firmware loader can read the firmware files
> > directly, and the traditional user-mode helper is invoked only as a
> > fallback.  This seems working pretty well, and the next step would be
> > to reduce the redundant user-mode helper stuff in future.
> >
> > This patch is a preparation for that: refactor the code for splitting
> > user-mode helper stuff more easily.  No functional change.
> 
> Looks a good code refactoring, :-)
> 
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_class.c | 286 +++++++++++++++++++++++-------------------
> >  1 file changed, 159 insertions(+), 127 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index b392b35..7da18e3 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -319,7 +319,8 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
> >         return true;
> >  }
> >
> > -static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> > +static bool fw_get_filesystem_firmware(struct device *device,
> > +                                      struct firmware_buf *buf)
> >  {
> >         int i;
> >         bool success = false;
> > @@ -343,6 +344,16 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
> >                         break;
> >         }
> >         __putname(path);
> > +
> > +       if (success) {
> > +               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> > +                       buf->fw_id);
> > +               mutex_lock(&fw_lock);
> > +               set_bit(FW_STATUS_DONE, &buf->status);
> > +               complete_all(&buf->completion);
> > +               mutex_unlock(&fw_lock);
> > +       }
> > +
> >         return success;
> >  }
> >
> > @@ -796,99 +807,122 @@ static int fw_add_devm_name(struct device *dev, const char *name)
> >  }
> >  #endif
> >
> > -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> > +static void _request_firmware_cleanup(struct firmware **firmware_p)
> >  {
> >         release_firmware(*firmware_p);
> >         *firmware_p = NULL;
> >  }
> >
> > -static struct firmware_priv *
> > -_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
> > -                         struct device *device, bool uevent, bool nowait)
> > +/* wait until the shared firmware_buf becomes ready (or error) */
> > +static int sync_cached_firmware_buf(struct firmware_buf *buf)
> > +{
> > +       int ret = 0;
> > +
> > +       mutex_lock(&fw_lock);
> > +       while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> > +               if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> > +                       ret = -ENOENT;
> > +                       break;
> > +               }
> > +               mutex_unlock(&fw_lock);
> > +               wait_for_completion(&buf->completion);
> > +               mutex_lock(&fw_lock);
> > +       }
> > +       mutex_unlock(&fw_lock);
> > +       return ret;
> > +}
> > +
> > +/* prepare firmware and firmware_buf structs;
> > + * return 1 if a firmware is already assigned, 0 if need to load one,
> > + * or a negative error code
> > + */
> > +static int
> > +_request_firmware_prepare(struct firmware **firmware_p, const char *name,
> > +                         struct device *device)
> >  {
> >         struct firmware *firmware;
> 
> Since the 'firmware_p' isn't marked as const, you can use it directly and
> remove the above local variable.

Yes, but the indirect access makes the code readability worse, IMO.
So I prefer keeping it.


> > -       struct firmware_priv *fw_priv = NULL;
> >         struct firmware_buf *buf;
> >         int ret;
> >
> >         if (!firmware_p)
> 
> Now the check isn't needed any more here, and it should be done in the entry
> of _request_firmware().

Right.


> > -               return ERR_PTR(-EINVAL);
> > +               return -EINVAL;
> >
> >         *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
> >         if (!firmware) {
> >                 dev_err(device, "%s: kmalloc(struct firmware) failed\n",
> >                         __func__);
> > -               return ERR_PTR(-ENOMEM);
> > +               return -ENOMEM;
> >         }
> >
> >         if (fw_get_builtin_firmware(firmware, name)) {
> >                 dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> > -               return NULL;
> > +               return 1; /* assigned */
> >         }
> >
> >         ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
> > -       if (!ret)
> > -               fw_priv = fw_create_instance(firmware, name, device,
> > -                               uevent, nowait);
> >
> > -       if (IS_ERR(fw_priv) || ret < 0) {
> > -               kfree(firmware);
> > -               *firmware_p = NULL;
> > -               return ERR_PTR(-ENOMEM);
> > -       } else if (fw_priv) {
> > -               fw_priv->buf = buf;
> > +       /*
> > +        * bind with 'buf' now to avoid warning in failure path
> > +        * of requesting firmware.
> > +        */
> > +       firmware->priv = buf;
> >
> > -               /*
> > -                * bind with 'buf' now to avoid warning in failure path
> > -                * of requesting firmware.
> > -                */
> > -               firmware->priv = buf;
> > -               return fw_priv;
> > +       if (ret > 0) {
> > +               ret = sync_cached_firmware_buf(buf);
> > +               if (!ret) {
> > +                       fw_set_page_data(buf, firmware);
> > +                       return 1; /* assigned */
> > +               }
> >         }
> >
> > -       /* share the cached buf, which is inprogessing or completed */
> > - check_status:
> > -       mutex_lock(&fw_lock);
> > -       if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> > -               fw_priv = ERR_PTR(-ENOENT);
> > -               firmware->priv = buf;
> > +       if (ret < 0)
> >                 _request_firmware_cleanup(firmware_p);
> 
> It is better to do cleanup firmware centralizedly in _request_firmware().

OK.

> > -               goto exit;
> > -       } else if (test_bit(FW_STATUS_DONE, &buf->status)) {
> > -               fw_priv = NULL;
> > -               fw_set_page_data(buf, firmware);
> > -               goto exit;
> > +
> > +       return ret;
> > +}
> > +
> > +static int assign_firmware_buf(struct firmware *fw, struct device *device)
> > +{
> > +       struct firmware_buf *buf = fw->priv;
> > +
> > +       mutex_lock(&fw_lock);
> > +       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) {
> > +               mutex_unlock(&fw_lock);
> > +               return -ENOENT;
> >         }
> > -       mutex_unlock(&fw_lock);
> > -       wait_for_completion(&buf->completion);
> > -       goto check_status;
> >
> > -exit:
> > +       /*
> > +        * add firmware name into devres list so that we can auto cache
> > +        * and uncache firmware for device.
> > +        *
> > +        * device may has been deleted already, but the problem
> > +        * should be fixed in devres or driver core.
> > +        */
> > +       if (device)
> > +               fw_add_devm_name(device, buf->fw_id);
> > +
> > +       /*
> > +        * After caching firmware image is started, let it piggyback
> > +        * on request firmware.
> > +        */
> > +       if (buf->fwc->state == FW_LOADER_START_CACHE) {
> > +               if (fw_cache_piggyback_on_request(buf->fw_id))
> > +                       kref_get(&buf->ref);
> > +       }
> > +
> > +       /* pass the pages buffer to driver at the last minute */
> > +       fw_set_page_data(buf, fw);
> >         mutex_unlock(&fw_lock);
> > -       return fw_priv;
> > +       return 0;
> >  }
> >
> > +/* load a firmware via user helper */
> >  static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> >                                   long timeout)
> >  {
> >         int retval = 0;
> >         struct device *f_dev = &fw_priv->dev;
> >         struct firmware_buf *buf = fw_priv->buf;
> > -       struct firmware_cache *fwc = &fw_cache;
> > -       int direct_load = 0;
> > -
> > -       /* try direct loading from fs first */
> > -       if (fw_get_filesystem_firmware(buf)) {
> > -               dev_dbg(f_dev->parent, "firmware: direct-loading"
> > -                       " firmware %s\n", buf->fw_id);
> > -
> > -               mutex_lock(&fw_lock);
> > -               set_bit(FW_STATUS_DONE, &buf->status);
> > -               mutex_unlock(&fw_lock);
> > -               complete_all(&buf->completion);
> > -               direct_load = 1;
> > -               goto handle_fw;
> > -       }
> >
> >         /* fall back on userspace loading */
> >         buf->fmt = PAGE_BUF;
> > @@ -929,38 +963,10 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> >
> >         cancel_delayed_work_sync(&fw_priv->timeout_work);
> >
> > -handle_fw:
> > -       mutex_lock(&fw_lock);
> > -       if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
> > -               retval = -ENOENT;
> > -
> > -       /*
> > -        * add firmware name into devres list so that we can auto cache
> > -        * and uncache firmware for device.
> > -        *
> > -        * f_dev->parent may has been deleted already, but the problem
> > -        * should be fixed in devres or driver core.
> > -        */
> > -       if (!retval && f_dev->parent)
> > -               fw_add_devm_name(f_dev->parent, buf->fw_id);
> > -
> > -       /*
> > -        * After caching firmware image is started, let it piggyback
> > -        * on request firmware.
> > -        */
> > -       if (!retval && fwc->state == FW_LOADER_START_CACHE) {
> > -               if (fw_cache_piggyback_on_request(buf->fw_id))
> > -                       kref_get(&buf->ref);
> > -       }
> > -
> > -       /* pass the pages buffer to driver at the last minute */
> > -       fw_set_page_data(buf, fw_priv->fw);
> > +       if (!retval)
> > +               retval = assign_firmware_buf(fw_priv->fw, f_dev->parent);
> >
> >         fw_priv->buf = NULL;
> > -       mutex_unlock(&fw_lock);
> > -
> > -       if (direct_load)
> > -               goto err_put_dev;
> >
> >         device_remove_file(f_dev, &dev_attr_loading);
> >  err_del_bin_attr:
> > @@ -972,6 +978,71 @@ err_put_dev:
> >         return retval;
> >  }
> >
> > +static int fw_load_from_user_helper(struct firmware *firmware,
> > +                                   const char *name, struct device *device,
> > +                                   bool uevent, bool nowait)
> > +{
> > +       struct firmware_priv *fw_priv;
> > +       long timeout;
> > +       int ret;
> > +
> > +       fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> > +       if (IS_ERR(fw_priv))
> > +               return PTR_ERR(fw_priv);
> > +
> > +       fw_priv->buf = firmware->priv;
> > +
> > +       timeout = firmware_loading_timeout();
> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       kfree(fw_priv);
> > +                       return -EAGAIN;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +                       kfree(fw_priv);
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       ret = _request_firmware_load(fw_priv, uevent, timeout);
> > +       usermodehelper_read_unlock();
> > +       return ret;
> > +}
> > +
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       int ret;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret < 0)
> > +               return ret;
> 
> It is better to handle the cleanup firmware outside _request_firmware_prepare,
> and 'goto' is preferred.

OK.

> > +       else if (ret) { /* assigned from either built-in or cache */
> > +               *firmware_p = fw;
> > +               return 0;
> 
> You might use 'goto' to do above.
> 
> > +       }
> > +
> > +       if (fw_get_filesystem_firmware(device, fw->priv))
> > +               ret = assign_firmware_buf(fw, device);
> > +       else
> > +               ret = fw_load_from_user_helper(fw, name, device,
> > +                                              uevent, nowait);
> 
> How about changing above as below and remove assign_firmware_buf()
> from _request_firmware_load()?
> 
> 	if (ret = !fw_get_filesystem_firmware(device, fw->priv))
> 		ret = fw_load_from_user_helper(fw, name, device,
> 						uevent, nowait);
> 
> 	if (!ret)
> 		ret = assign_firmware_buf(fw, device);
> 
> So we can assign buffer only in one place.

Sounds reasonable.  I thought some race in user-helper mode loading,
but apparently it's OK to call this function at a later point.



thanks,

Takashi


> > +       if (ret)
> > +               _request_firmware_cleanup(&fw);
> > +       *firmware_p = fw;
> > +       return ret;
> > +}
> > +
> >  /**
> >   * request_firmware: - send firmware request and wait for it
> >   * @firmware_p: pointer to firmware image
> > @@ -996,26 +1067,7 @@ int
> >  request_firmware(const struct firmware **firmware_p, const char *name,
> >                   struct device *device)
> >  {
> > -       struct firmware_priv *fw_priv;
> > -       int ret;
> > -
> > -       fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
> > -                                           false);
> > -       if (IS_ERR_OR_NULL(fw_priv))
> > -               return PTR_RET(fw_priv);
> > -
> > -       ret = usermodehelper_read_trylock();
> > -       if (WARN_ON(ret)) {
> > -               dev_err(device, "firmware: %s will not be loaded\n", name);
> > -       } else {
> > -               ret = _request_firmware_load(fw_priv, true,
> > -                                       firmware_loading_timeout());
> > -               usermodehelper_read_unlock();
> > -       }
> > -       if (ret)
> > -               _request_firmware_cleanup(firmware_p);
> > -
> > -       return ret;
> > +       return _request_firmware(firmware_p, name, device, true, false);
> >  }
> >
> >  /**
> > @@ -1046,31 +1098,11 @@ static void request_firmware_work_func(struct work_struct *work)
> >  {
> >         struct firmware_work *fw_work;
> >         const struct firmware *fw;
> > -       struct firmware_priv *fw_priv;
> > -       long timeout;
> > -       int ret;
> >
> >         fw_work = container_of(work, struct firmware_work, work);
> > -       fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
> > -                       fw_work->uevent, true);
> > -       if (IS_ERR_OR_NULL(fw_priv)) {
> > -               ret = PTR_RET(fw_priv);
> > -               goto out;
> > -       }
> > -
> > -       timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
> > -       if (timeout) {
> > -               ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout);
> > -               usermodehelper_read_unlock();
> > -       } else {
> > -               dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
> > -                       fw_work->name);
> > -               ret = -EAGAIN;
> > -       }
> > -       if (ret)
> > -               _request_firmware_cleanup(&fw);
> >
> > - out:
> > +       _request_firmware(&fw, fw_work->name, fw_work->device,
> > +                         fw_work->uevent, true);
> >         fw_work->cont(fw, fw_work->context);
> >         put_device(fw_work->device);
> >
> 
> Thanks,
> --
> Ming Lei
>