[RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

Hi,

In [1][2], the problem below has been discussed for some time:

	device's firmware may be lost during suspend/resume
	cycle because device is unplugged and plugged again
	or device experiences system power loss in the period.
	but during resume path, system is still not ready(process
	frozen, rootfs not usable, ...) to complete loading firmware
	from user space for the device

The conclusion is that caching firmware during suspend/resume cycle
is capable of solving the problem.

This patchset implements cache/uncache firmware mechanism,
and apply the mechnism to cache device's firmware in kernel memory
space automatically during suspend/resume cyclye, so device can
load its firmware easily during resume path. When resume is completed
and system is ready, the cached firmwares will be removed from
kernel memory later.

Even there are some corener cases[3] which can't be solved this cache
approach, but as Linus pointed, the driver should use some specific
way to fix it, for example, the isight camera problem can easily be
solved with deferral probe by driver explicitly, see [1], and some
patches will be posted later to do it.


[1]. http://marc.info/?t=134278790800004&r=1&w=2
[2]. http://marc.info/?t=132528956000002&r=10&w=2
[3]. http://marc.info/?l=linux-usb&m=132554118928398&w=2

Thanks,
--
Ming Lei

[RFC PATCH 01/13] driver core: firmware loader: simplify pages ownership transfer

[Ming Lei]

This patch doesn't transfer ownership of pages' buffer to the
instance of firmware until the firmware loading is completed,
which will simplify firmware_loading_store a lot, so help
to introduce the following cache_firmware and uncache_firmware
mechanism during system suspend-resume cycle.

In fact, this patch fixes one bug: if writing data into
firmware loader device is bypassed between writting 1 and 0 to
'loading', OOPS will be triggered without the patch.

Also add some comments to make code more readable.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   57 ++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 803cfc1..f789bbd 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -93,6 +93,8 @@ struct firmware_priv {
 	struct completion completion;
 	struct firmware *fw;
 	unsigned long status;
+	void *data;
+	size_t size;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
@@ -156,9 +158,11 @@ static void fw_dev_release(struct device *dev)
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int i;
 
+	/* free untransfered pages buffer */
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kfree(fw_priv->pages);
+
 	kfree(fw_priv);
 
 	module_put(THIS_MODULE);
@@ -194,6 +198,7 @@ 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)
 {
 	int i;
@@ -237,9 +242,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 
 	switch (loading) {
 	case 1:
-		firmware_free_data(fw_priv->fw);
-		memset(fw_priv->fw, 0, sizeof(struct firmware));
-		/* If the pages are not owned by 'struct firmware' */
+		/* discarding any previous partial load */
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
 		kfree(fw_priv->pages);
@@ -250,20 +253,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			vunmap(fw_priv->fw->data);
-			fw_priv->fw->data = vmap(fw_priv->pages,
-						 fw_priv->nr_pages,
-						 0, PAGE_KERNEL_RO);
-			if (!fw_priv->fw->data) {
-				dev_err(dev, "%s: vmap() failed\n", __func__);
-				goto err;
-			}
-			/* Pages are now owned by 'struct firmware' */
-			fw_priv->fw->pages = fw_priv->pages;
-			fw_priv->pages = NULL;
-
-			fw_priv->page_array_size = 0;
-			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
 			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
 			break;
@@ -273,7 +262,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-	err:
 		fw_load_abort(fw_priv);
 		break;
 	}
@@ -299,12 +287,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 		ret_count = -ENODEV;
 		goto out;
 	}
-	if (offset > fw->size) {
+	if (offset > fw_priv->size) {
 		ret_count = 0;
 		goto out;
 	}
-	if (count > fw->size - offset)
-		count = fw->size - offset;
+	if (count > fw_priv->size - offset)
+		count = fw_priv->size - offset;
 
 	ret_count = count;
 
@@ -396,6 +384,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		retval = -ENODEV;
 		goto out;
 	}
+
 	retval = fw_realloc_buffer(fw_priv, offset + count);
 	if (retval)
 		goto out;
@@ -418,7 +407,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		count -= page_cnt;
 	}
 
-	fw->size = max_t(size_t, offset, fw->size);
+	fw_priv->size = max_t(size_t, offset, fw_priv->size);
 out:
 	mutex_unlock(&fw_lock);
 	return retval;
@@ -504,6 +493,24 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p)
 	*firmware_p = NULL;
 }
 
+/* transfer the ownership of pages to firmware */
+static void fw_set_page_data(struct firmware_priv *fw_priv)
+{
+	struct firmware *fw = fw_priv->fw;
+
+	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages,
+				0, PAGE_KERNEL_RO);
+	fw->data = fw_priv->data;
+	fw->pages = fw_priv->pages;
+	fw->size = fw_priv->size;
+
+	WARN_ON(PFN_UP(fw->size) != fw_priv->nr_pages);
+
+	fw_priv->nr_pages = 0;
+	fw_priv->pages = NULL;
+	fw_priv->data = NULL;
+}
+
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 				  long timeout)
 {
@@ -549,8 +556,12 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	del_timer_sync(&fw_priv->timeout);
 
 	mutex_lock(&fw_lock);
-	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status))
+	if (!fw_priv->size || test_bit(FW_STATUS_ABORT, &fw_priv->status))
 		retval = -ENOENT;
+
+	/* transfer pages ownership at the last minute */
+	if (!retval)
+		fw_set_page_data(fw_priv);
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);
 
-- 
1.7.9.5

Re: [RFC PATCH 01/13] driver core: firmware loader: simplify pages ownership transfer

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:01AM +0800, Ming Lei wrote:
> This patch doesn't transfer ownership of pages' buffer to the
> instance of firmware until the firmware loading is completed,
> which will simplify firmware_loading_store a lot, so help
> to introduce the following cache_firmware and uncache_firmware
> mechanism during system suspend-resume cycle.
> 
> In fact, this patch fixes one bug: if writing data into
> firmware loader device is bypassed between writting 1 and 0 to
> 'loading', OOPS will be triggered without the patch.
> 
> Also add some comments to make code more readable.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |   57 ++++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 803cfc1..f789bbd 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -93,6 +93,8 @@ struct firmware_priv {
>  	struct completion completion;
>  	struct firmware *fw;
>  	unsigned long status;
> +	void *data;
> +	size_t size;
>  	struct page **pages;
>  	int nr_pages;
>  	int page_array_size;
> @@ -156,9 +158,11 @@ static void fw_dev_release(struct device *dev)
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  	int i;
>  
> +	/* free untransfered pages buffer */
>  	for (i = 0; i < fw_priv->nr_pages; i++)
>  		__free_page(fw_priv->pages[i]);
>  	kfree(fw_priv->pages);
> +
>  	kfree(fw_priv);
>  
>  	module_put(THIS_MODULE);
> @@ -194,6 +198,7 @@ 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)
>  {
>  	int i;
> @@ -237,9 +242,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  
>  	switch (loading) {
>  	case 1:
> -		firmware_free_data(fw_priv->fw);
> -		memset(fw_priv->fw, 0, sizeof(struct firmware));
> -		/* If the pages are not owned by 'struct firmware' */
> +		/* discarding any previous partial load */
>  		for (i = 0; i < fw_priv->nr_pages; i++)
>  			__free_page(fw_priv->pages[i]);
>  		kfree(fw_priv->pages);
> @@ -250,20 +253,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		break;
>  	case 0:
>  		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
> -			vunmap(fw_priv->fw->data);
> -			fw_priv->fw->data = vmap(fw_priv->pages,
> -						 fw_priv->nr_pages,
> -						 0, PAGE_KERNEL_RO);
> -			if (!fw_priv->fw->data) {
> -				dev_err(dev, "%s: vmap() failed\n", __func__);
> -				goto err;
> -			}
> -			/* Pages are now owned by 'struct firmware' */
> -			fw_priv->fw->pages = fw_priv->pages;
> -			fw_priv->pages = NULL;
> -
> -			fw_priv->page_array_size = 0;
> -			fw_priv->nr_pages = 0;
>  			complete(&fw_priv->completion);
>  			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
>  			break;
> @@ -273,7 +262,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
>  		/* fallthrough */
>  	case -1:
> -	err:
>  		fw_load_abort(fw_priv);
>  		break;
>  	}
> @@ -299,12 +287,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> -	if (offset > fw->size) {
> +	if (offset > fw_priv->size) {
>  		ret_count = 0;
>  		goto out;
>  	}
> -	if (count > fw->size - offset)
> -		count = fw->size - offset;
> +	if (count > fw_priv->size - offset)
> +		count = fw_priv->size - offset;
>  
>  	ret_count = count;
>  
> @@ -396,6 +384,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  		retval = -ENODEV;
>  		goto out;
>  	}
> +
>  	retval = fw_realloc_buffer(fw_priv, offset + count);
>  	if (retval)
>  		goto out;
> @@ -418,7 +407,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  		count -= page_cnt;
>  	}
>  
> -	fw->size = max_t(size_t, offset, fw->size);
> +	fw_priv->size = max_t(size_t, offset, fw_priv->size);
>  out:
>  	mutex_unlock(&fw_lock);
>  	return retval;
> @@ -504,6 +493,24 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p)
>  	*firmware_p = NULL;
>  }
>  
> +/* transfer the ownership of pages to firmware */
> +static void fw_set_page_data(struct firmware_priv *fw_priv)
> +{
> +	struct firmware *fw = fw_priv->fw;
> +
> +	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages,
> +				0, PAGE_KERNEL_RO);

We don't need to check the return value of vmap() here like we do above?

> +	fw->data = fw_priv->data;
> +	fw->pages = fw_priv->pages;
> +	fw->size = fw_priv->size;

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 01/13] driver core: firmware loader: simplify pages ownership transfer

[Ming Lei]

On Wed, Jul 25, 2012 at 2:10 AM, Borislav Petkov <bp@amd64.org> wrote:
>>
>> +/* transfer the ownership of pages to firmware */
>> +static void fw_set_page_data(struct firmware_priv *fw_priv)
>> +{
>> +     struct firmware *fw = fw_priv->fw;
>> +
>> +     fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages,
>> +                             0, PAGE_KERNEL_RO);
>
> We don't need to check the return value of vmap() here like we do above?

Good catch, it should be handled, otherwise may cause oops inside driver.

If that happens, the pages buffer should be dropped and return failure
from request_firmware or the callback of request_firmware_nowait.

I will fix it against this patch set if cache/uncache firmware can be
accepted.

Thanks,
--
Ming Lei

[RFC PATCH 02/13] driver core: firmware loader: fix races during loading firmware

[Ming Lei]

This patch fixes two races in loading firmware:

1, FW_STATUS_DONE should be set before waking up the task waitting
on _request_firmware_load, otherwise FW_STATUS_ABORT may be
thought as DONE mistakenly.

2, Inside _request_firmware_load(), there is a small window between
wait_for_completion() and mutex_lock(&fw_lock), and 'echo 1 > loading'
still may happen during the period, so this patch checks FW_STATUS_DONE
to prevent pages' buffer completed from being freed in firmware_loading_store.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f789bbd..6e96b8c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -243,18 +243,21 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		for (i = 0; i < fw_priv->nr_pages; i++)
-			__free_page(fw_priv->pages[i]);
-		kfree(fw_priv->pages);
-		fw_priv->pages = NULL;
-		fw_priv->page_array_size = 0;
-		fw_priv->nr_pages = 0;
-		set_bit(FW_STATUS_LOADING, &fw_priv->status);
+		if (!test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+			for (i = 0; i < fw_priv->nr_pages; i++)
+				__free_page(fw_priv->pages[i]);
+			kfree(fw_priv->pages);
+			fw_priv->pages = NULL;
+			fw_priv->page_array_size = 0;
+			fw_priv->nr_pages = 0;
+			set_bit(FW_STATUS_LOADING, &fw_priv->status);
+		}
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			complete(&fw_priv->completion);
+			set_bit(FW_STATUS_DONE, &fw_priv->status);
 			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
+			complete(&fw_priv->completion);
 			break;
 		}
 		/* fallthrough */
@@ -552,7 +555,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	wait_for_completion(&fw_priv->completion);
 
-	set_bit(FW_STATUS_DONE, &fw_priv->status);
 	del_timer_sync(&fw_priv->timeout);
 
 	mutex_lock(&fw_lock);
-- 
1.7.9.5

[RFC PATCH 03/13] driver core: firmware loader: remove unnecessary wmb()

[Ming Lei]

The wmb() inside fw_load_abort is not necessary, since
complete() and wait_on_completion() has implied one pair
of memory barrier.

Also wmb() isn't a correct usage, so just remove it.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6e96b8c..16239fd 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -112,7 +112,6 @@ static struct firmware_priv *to_firmware_priv(struct device *dev)
 static void fw_load_abort(struct firmware_priv *fw_priv)
 {
 	set_bit(FW_STATUS_ABORT, &fw_priv->status);
-	wmb();
 	complete(&fw_priv->completion);
 }
 
-- 
1.7.9.5

[RFC PATCH 04/13] driver core: firmware loader: fix creation failure of fw loader device

[Ming Lei]

If one device driver calls request_firmware_nowait() to request
several different firmwares' loading, device_add() will return
failure since all firmware loader device use same name of the
device who is requesting firmware.

This patch always use the name of firmware image as the firmware
loader device name to fix the problem since the following patches
for caching firmware will make sure only one loading for same
firmware is alllowd at the same time.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 16239fd..0b343b8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -452,7 +452,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	f_dev = &fw_priv->dev;
 
 	device_initialize(f_dev);
-	dev_set_name(f_dev, "%s", dev_name(device));
+	dev_set_name(f_dev, "%s", fw_name);
 	f_dev->parent = device;
 	f_dev->class = &firmware_class;
 
-- 
1.7.9.5

[RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf

[Ming Lei]

This patch introduces struct firmware_buf to describe the buffer
which holds the firmware data, which will make the following
cache_firmware/uncache_firmware implemented easily.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |  176 +++++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 75 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 0b343b8..986d9df 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -89,7 +89,7 @@ static inline long firmware_loading_timeout(void)
  * guarding for corner cases a global lock should be OK */
 static DEFINE_MUTEX(fw_lock);
 
-struct firmware_priv {
+struct firmware_buf {
 	struct completion completion;
 	struct firmware *fw;
 	unsigned long status;
@@ -98,10 +98,14 @@ struct firmware_priv {
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
+	char fw_id[];
+};
+
+struct firmware_priv {
 	struct timer_list timeout;
-	struct device dev;
 	bool nowait;
-	char fw_id[];
+	struct device dev;
+	struct firmware_buf *buf;
 };
 
 static struct firmware_priv *to_firmware_priv(struct device *dev)
@@ -111,8 +115,10 @@ static struct firmware_priv *to_firmware_priv(struct device *dev)
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
 {
-	set_bit(FW_STATUS_ABORT, &fw_priv->status);
-	complete(&fw_priv->completion);
+	struct firmware_buf *buf = fw_priv->buf;
+
+	set_bit(FW_STATUS_ABORT, &buf->status);
+	complete(&buf->completion);
 }
 
 static ssize_t firmware_timeout_show(struct class *class,
@@ -152,16 +158,23 @@ static struct class_attribute firmware_class_attrs[] = {
 	__ATTR_NULL
 };
 
-static void fw_dev_release(struct device *dev)
+static void fw_free_buf(struct firmware_buf *buf)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int i;
 
-	/* free untransfered pages buffer */
-	for (i = 0; i < fw_priv->nr_pages; i++)
-		__free_page(fw_priv->pages[i]);
-	kfree(fw_priv->pages);
+	if (!buf)
+		return;
+
+	for (i = 0; i < buf->nr_pages; i++)
+		__free_page(buf->pages[i]);
+	kfree(buf->pages);
+}
+
+static void fw_dev_release(struct device *dev)
+{
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
+	kfree(fw_priv->buf);
 	kfree(fw_priv);
 
 	module_put(THIS_MODULE);
@@ -171,7 +184,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
-	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->fw_id))
+	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
@@ -192,7 +205,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status);
+	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -231,32 +244,33 @@ static ssize_t firmware_loading_store(struct device *dev,
 				      const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct firmware_buf *fw_buf = fw_priv->buf;
 	int loading = simple_strtol(buf, NULL, 10);
 	int i;
 
 	mutex_lock(&fw_lock);
 
-	if (!fw_priv->fw)
+	if (!fw_buf)
 		goto out;
 
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_priv->status)) {
-			for (i = 0; i < fw_priv->nr_pages; i++)
-				__free_page(fw_priv->pages[i]);
-			kfree(fw_priv->pages);
-			fw_priv->pages = NULL;
-			fw_priv->page_array_size = 0;
-			fw_priv->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_priv->status);
+		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+			for (i = 0; i < fw_buf->nr_pages; i++)
+				__free_page(fw_buf->pages[i]);
+			kfree(fw_buf->pages);
+			fw_buf->pages = NULL;
+			fw_buf->page_array_size = 0;
+			fw_buf->nr_pages = 0;
+			set_bit(FW_STATUS_LOADING, &fw_buf->status);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			set_bit(FW_STATUS_DONE, &fw_priv->status);
-			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
-			complete(&fw_priv->completion);
+		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+			set_bit(FW_STATUS_DONE, &fw_buf->status);
+			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
+			complete(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -280,21 +294,21 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-	struct firmware *fw;
+	struct firmware_buf *buf;
 	ssize_t ret_count;
 
 	mutex_lock(&fw_lock);
-	fw = fw_priv->fw;
-	if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+	buf = fw_priv->buf;
+	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
-	if (offset > fw_priv->size) {
+	if (offset > buf->size) {
 		ret_count = 0;
 		goto out;
 	}
-	if (count > fw_priv->size - offset)
-		count = fw_priv->size - offset;
+	if (count > buf->size - offset)
+		count = buf->size - offset;
 
 	ret_count = count;
 
@@ -304,11 +318,11 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 		int page_ofs = offset & (PAGE_SIZE-1);
 		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
 
-		page_data = kmap(fw_priv->pages[page_nr]);
+		page_data = kmap(buf->pages[page_nr]);
 
 		memcpy(buffer, page_data + page_ofs, page_cnt);
 
-		kunmap(fw_priv->pages[page_nr]);
+		kunmap(buf->pages[page_nr]);
 		buffer += page_cnt;
 		offset += page_cnt;
 		count -= page_cnt;
@@ -320,12 +334,13 @@ out:
 
 static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 {
+	struct firmware_buf *buf = fw_priv->buf;
 	int pages_needed = ALIGN(min_size, PAGE_SIZE) >> PAGE_SHIFT;
 
 	/* If the array of pages is too small, grow it... */
-	if (fw_priv->page_array_size < pages_needed) {
+	if (buf->page_array_size < pages_needed) {
 		int new_array_size = max(pages_needed,
-					 fw_priv->page_array_size * 2);
+					 buf->page_array_size * 2);
 		struct page **new_pages;
 
 		new_pages = kmalloc(new_array_size * sizeof(void *),
@@ -334,24 +349,24 @@ static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 			fw_load_abort(fw_priv);
 			return -ENOMEM;
 		}
-		memcpy(new_pages, fw_priv->pages,
-		       fw_priv->page_array_size * sizeof(void *));
-		memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) *
-		       (new_array_size - fw_priv->page_array_size));
-		kfree(fw_priv->pages);
-		fw_priv->pages = new_pages;
-		fw_priv->page_array_size = new_array_size;
+		memcpy(new_pages, buf->pages,
+		       buf->page_array_size * sizeof(void *));
+		memset(&new_pages[buf->page_array_size], 0, sizeof(void *) *
+		       (new_array_size - buf->page_array_size));
+		kfree(buf->pages);
+		buf->pages = new_pages;
+		buf->page_array_size = new_array_size;
 	}
 
-	while (fw_priv->nr_pages < pages_needed) {
-		fw_priv->pages[fw_priv->nr_pages] =
+	while (buf->nr_pages < pages_needed) {
+		buf->pages[buf->nr_pages] =
 			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
 
-		if (!fw_priv->pages[fw_priv->nr_pages]) {
+		if (!buf->pages[buf->nr_pages]) {
 			fw_load_abort(fw_priv);
 			return -ENOMEM;
 		}
-		fw_priv->nr_pages++;
+		buf->nr_pages++;
 	}
 	return 0;
 }
@@ -374,15 +389,15 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-	struct firmware *fw;
+	struct firmware_buf *buf;
 	ssize_t retval;
 
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
 
 	mutex_lock(&fw_lock);
-	fw = fw_priv->fw;
-	if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+	buf = fw_priv->buf;
+	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -399,17 +414,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		int page_ofs = offset & (PAGE_SIZE - 1);
 		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
 
-		page_data = kmap(fw_priv->pages[page_nr]);
+		page_data = kmap(buf->pages[page_nr]);
 
 		memcpy(page_data + page_ofs, buffer, page_cnt);
 
-		kunmap(fw_priv->pages[page_nr]);
+		kunmap(buf->pages[page_nr]);
 		buffer += page_cnt;
 		offset += page_cnt;
 		count -= page_cnt;
 	}
 
-	fw_priv->size = max_t(size_t, offset, fw_priv->size);
+	buf->size = max_t(size_t, offset, buf->size);
 out:
 	mutex_unlock(&fw_lock);
 	return retval;
@@ -434,20 +449,31 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 		   struct device *device, bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;
+	struct firmware_buf *buf;
 	struct device *f_dev;
 
-	fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
+	fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL);
 	if (!fw_priv) {
 		dev_err(device, "%s: kmalloc failed\n", __func__);
-		return ERR_PTR(-ENOMEM);
+		fw_priv = ERR_PTR(-ENOMEM);
+		goto exit;
 	}
 
-	fw_priv->fw = firmware;
+	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_KERNEL);
+	if (!buf) {
+		dev_err(device, "%s: kmalloc failed\n", __func__);
+		kfree(fw_priv);
+		fw_priv = ERR_PTR(-ENOMEM);
+		goto exit;
+	}
+
+	buf->fw = firmware;
+	fw_priv->buf = buf;
 	fw_priv->nowait = nowait;
-	strcpy(fw_priv->fw_id, fw_name);
-	init_completion(&fw_priv->completion);
 	setup_timer(&fw_priv->timeout,
 		    firmware_class_timeout, (u_long) fw_priv);
+	strcpy(buf->fw_id, fw_name);
+	init_completion(&buf->completion);
 
 	f_dev = &fw_priv->dev;
 
@@ -455,7 +481,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	dev_set_name(f_dev, "%s", fw_name);
 	f_dev->parent = device;
 	f_dev->class = &firmware_class;
-
+exit:
 	return fw_priv;
 }
 
@@ -496,21 +522,17 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p)
 }
 
 /* transfer the ownership of pages to firmware */
-static void fw_set_page_data(struct firmware_priv *fw_priv)
+static void fw_set_page_data(struct firmware_buf *buf)
 {
-	struct firmware *fw = fw_priv->fw;
+	struct firmware *fw = buf->fw;
 
-	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages,
+	buf->data = vmap(buf->pages, buf->nr_pages,
 				0, PAGE_KERNEL_RO);
-	fw->data = fw_priv->data;
-	fw->pages = fw_priv->pages;
-	fw->size = fw_priv->size;
-
-	WARN_ON(PFN_UP(fw->size) != fw_priv->nr_pages);
+	fw->data = buf->data;
+	fw->pages = buf->pages;
+	fw->size = buf->size;
 
-	fw_priv->nr_pages = 0;
-	fw_priv->pages = NULL;
-	fw_priv->data = NULL;
+	WARN_ON(PFN_UP(fw->size) != buf->nr_pages);
 }
 
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
@@ -518,6 +540,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
+	struct firmware_buf *buf = fw_priv->buf;
 
 	dev_set_uevent_suppress(f_dev, true);
 
@@ -544,7 +567,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	if (uevent) {
 		dev_set_uevent_suppress(f_dev, false);
-		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_id);
+		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
 		if (timeout != MAX_SCHEDULE_TIMEOUT)
 			mod_timer(&fw_priv->timeout,
 				  round_jiffies_up(jiffies + timeout));
@@ -552,18 +575,21 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
 	}
 
-	wait_for_completion(&fw_priv->completion);
+	wait_for_completion(&buf->completion);
 
 	del_timer_sync(&fw_priv->timeout);
 
 	mutex_lock(&fw_lock);
-	if (!fw_priv->size || test_bit(FW_STATUS_ABORT, &fw_priv->status))
+	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
 		retval = -ENOENT;
 
 	/* transfer pages ownership at the last minute */
 	if (!retval)
-		fw_set_page_data(fw_priv);
-	fw_priv->fw = NULL;
+		fw_set_page_data(buf);
+	else
+		fw_free_buf(buf); /* free untransfered pages buffer */
+
+	fw_priv->buf = NULL;
 	mutex_unlock(&fw_lock);
 
 	device_remove_file(f_dev, &dev_attr_loading);
-- 
1.7.9.5

Re: [RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:05AM +0800, Ming Lei wrote:
> This patch introduces struct firmware_buf to describe the buffer
> which holds the firmware data, which will make the following
> cache_firmware/uncache_firmware implemented easily.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |  176 +++++++++++++++++++++++------------------
>  1 file changed, 101 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 0b343b8..986d9df 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -89,7 +89,7 @@ static inline long firmware_loading_timeout(void)
>   * guarding for corner cases a global lock should be OK */
>  static DEFINE_MUTEX(fw_lock);
>  
> -struct firmware_priv {
> +struct firmware_buf {
>  	struct completion completion;
>  	struct firmware *fw;
>  	unsigned long status;
> @@ -98,10 +98,14 @@ struct firmware_priv {
>  	struct page **pages;
>  	int nr_pages;
>  	int page_array_size;
> +	char fw_id[];
> +};
> +
> +struct firmware_priv {
>  	struct timer_list timeout;
> -	struct device dev;
>  	bool nowait;
> -	char fw_id[];
> +	struct device dev;
> +	struct firmware_buf *buf;
>  };
>  
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
> @@ -111,8 +115,10 @@ static struct firmware_priv *to_firmware_priv(struct device *dev)
>  
>  static void fw_load_abort(struct firmware_priv *fw_priv)
>  {
> -	set_bit(FW_STATUS_ABORT, &fw_priv->status);
> -	complete(&fw_priv->completion);
> +	struct firmware_buf *buf = fw_priv->buf;
> +
> +	set_bit(FW_STATUS_ABORT, &buf->status);
> +	complete(&buf->completion);
>  }
>  
>  static ssize_t firmware_timeout_show(struct class *class,
> @@ -152,16 +158,23 @@ static struct class_attribute firmware_class_attrs[] = {
>  	__ATTR_NULL
>  };
>  
> -static void fw_dev_release(struct device *dev)
> +static void fw_free_buf(struct firmware_buf *buf)
>  {
> -	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  	int i;
>  
> -	/* free untransfered pages buffer */
> -	for (i = 0; i < fw_priv->nr_pages; i++)
> -		__free_page(fw_priv->pages[i]);
> -	kfree(fw_priv->pages);
> +	if (!buf)
> +		return;

This is subtle: the caller of fw_free_buf might forget to assign NULL to
the buf ptr.

Why not pass struct firmware_priv *fw_priv to the function instead and ...

> +
> +	for (i = 0; i < buf->nr_pages; i++)
> +		__free_page(buf->pages[i]);
> +	kfree(buf->pages);

assign NULL to the ptr as a last step, when all is done:

	fw_priv->buf = NULL;

This way you're making sure ->buf is NULL after all pages are freed and
your check above is always correct.

> +}
> +
> +static void fw_dev_release(struct device *dev)
> +{
> +	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  
> +	kfree(fw_priv->buf);
>  	kfree(fw_priv);
>  
>  	module_put(THIS_MODULE);
> @@ -171,7 +184,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  
> -	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->fw_id))
> +	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id))
>  		return -ENOMEM;
>  	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
>  		return -ENOMEM;
> @@ -192,7 +205,7 @@ static ssize_t firmware_loading_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> -	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status);
> +	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
>  
>  	return sprintf(buf, "%d\n", loading);
>  }
> @@ -231,32 +244,33 @@ static ssize_t firmware_loading_store(struct device *dev,
>  				      const char *buf, size_t count)
>  {
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +	struct firmware_buf *fw_buf = fw_priv->buf;
>  	int loading = simple_strtol(buf, NULL, 10);
>  	int i;
>  
>  	mutex_lock(&fw_lock);
>  
> -	if (!fw_priv->fw)
> +	if (!fw_buf)
>  		goto out;
>  
>  	switch (loading) {
>  	case 1:
>  		/* discarding any previous partial load */
> -		if (!test_bit(FW_STATUS_DONE, &fw_priv->status)) {
> -			for (i = 0; i < fw_priv->nr_pages; i++)
> -				__free_page(fw_priv->pages[i]);
> -			kfree(fw_priv->pages);
> -			fw_priv->pages = NULL;
> -			fw_priv->page_array_size = 0;
> -			fw_priv->nr_pages = 0;
> -			set_bit(FW_STATUS_LOADING, &fw_priv->status);
> +		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +			for (i = 0; i < fw_buf->nr_pages; i++)
> +				__free_page(fw_buf->pages[i]);
> +			kfree(fw_buf->pages);
> +			fw_buf->pages = NULL;
> +			fw_buf->page_array_size = 0;
> +			fw_buf->nr_pages = 0;
> +			set_bit(FW_STATUS_LOADING, &fw_buf->status);
>  		}
>  		break;
>  	case 0:
> -		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
> -			set_bit(FW_STATUS_DONE, &fw_priv->status);
> -			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
> -			complete(&fw_priv->completion);
> +		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +			set_bit(FW_STATUS_DONE, &fw_buf->status);
> +			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> +			complete(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -280,21 +294,21 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> -	struct firmware *fw;
> +	struct firmware_buf *buf;
>  	ssize_t ret_count;
>  
>  	mutex_lock(&fw_lock);
> -	fw = fw_priv->fw;
> -	if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) {
> +	buf = fw_priv->buf;
> +	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> -	if (offset > fw_priv->size) {
> +	if (offset > buf->size) {
>  		ret_count = 0;
>  		goto out;
>  	}
> -	if (count > fw_priv->size - offset)
> -		count = fw_priv->size - offset;
> +	if (count > buf->size - offset)
> +		count = buf->size - offset;
>  
>  	ret_count = count;
>  
> @@ -304,11 +318,11 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  		int page_ofs = offset & (PAGE_SIZE-1);
>  		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
>  
> -		page_data = kmap(fw_priv->pages[page_nr]);
> +		page_data = kmap(buf->pages[page_nr]);
>  
>  		memcpy(buffer, page_data + page_ofs, page_cnt);
>  
> -		kunmap(fw_priv->pages[page_nr]);
> +		kunmap(buf->pages[page_nr]);
>  		buffer += page_cnt;
>  		offset += page_cnt;
>  		count -= page_cnt;
> @@ -320,12 +334,13 @@ out:

While you're at it, you can indent this "out:" label one space to the
right so that the diff can pick up the function name in the hunk tag
above instead of the label.

@@ -329,7 +329,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
                offset += page_cnt;
                count -= page_cnt;
        }
-out:
+ out:
        mutex_unlock(&fw_lock);
        return ret_count;
 }

>  static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
>  {
> +	struct firmware_buf *buf = fw_priv->buf;
>  	int pages_needed = ALIGN(min_size, PAGE_SIZE) >> PAGE_SHIFT;
>  
>  	/* If the array of pages is too small, grow it... */
> -	if (fw_priv->page_array_size < pages_needed) {
> +	if (buf->page_array_size < pages_needed) {
>  		int new_array_size = max(pages_needed,
> -					 fw_priv->page_array_size * 2);
> +					 buf->page_array_size * 2);
>  		struct page **new_pages;
>  
>  		new_pages = kmalloc(new_array_size * sizeof(void *),
> @@ -334,24 +349,24 @@ static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
>  			fw_load_abort(fw_priv);
>  			return -ENOMEM;
>  		}
> -		memcpy(new_pages, fw_priv->pages,
> -		       fw_priv->page_array_size * sizeof(void *));
> -		memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) *
> -		       (new_array_size - fw_priv->page_array_size));
> -		kfree(fw_priv->pages);
> -		fw_priv->pages = new_pages;
> -		fw_priv->page_array_size = new_array_size;
> +		memcpy(new_pages, buf->pages,
> +		       buf->page_array_size * sizeof(void *));
> +		memset(&new_pages[buf->page_array_size], 0, sizeof(void *) *
> +		       (new_array_size - buf->page_array_size));
> +		kfree(buf->pages);
> +		buf->pages = new_pages;
> +		buf->page_array_size = new_array_size;
>  	}
>  
> -	while (fw_priv->nr_pages < pages_needed) {
> -		fw_priv->pages[fw_priv->nr_pages] =
> +	while (buf->nr_pages < pages_needed) {
> +		buf->pages[buf->nr_pages] =
>  			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
>  
> -		if (!fw_priv->pages[fw_priv->nr_pages]) {
> +		if (!buf->pages[buf->nr_pages]) {
>  			fw_load_abort(fw_priv);
>  			return -ENOMEM;
>  		}
> -		fw_priv->nr_pages++;
> +		buf->nr_pages++;
>  	}
>  	return 0;
>  }
> @@ -374,15 +389,15 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> -	struct firmware *fw;
> +	struct firmware_buf *buf;
>  	ssize_t retval;
>  
>  	if (!capable(CAP_SYS_RAWIO))
>  		return -EPERM;
>  
>  	mutex_lock(&fw_lock);
> -	fw = fw_priv->fw;
> -	if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) {
> +	buf = fw_priv->buf;
> +	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
>  		retval = -ENODEV;
>  		goto out;
>  	}
> @@ -399,17 +414,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  		int page_ofs = offset & (PAGE_SIZE - 1);
>  		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
>  
> -		page_data = kmap(fw_priv->pages[page_nr]);
> +		page_data = kmap(buf->pages[page_nr]);
>  
>  		memcpy(page_data + page_ofs, buffer, page_cnt);
>  
> -		kunmap(fw_priv->pages[page_nr]);
> +		kunmap(buf->pages[page_nr]);
>  		buffer += page_cnt;
>  		offset += page_cnt;
>  		count -= page_cnt;
>  	}
>  
> -	fw_priv->size = max_t(size_t, offset, fw_priv->size);
> +	buf->size = max_t(size_t, offset, buf->size);
>  out:
>  	mutex_unlock(&fw_lock);
>  	return retval;
> @@ -434,20 +449,31 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>  		   struct device *device, bool uevent, bool nowait)
>  {
>  	struct firmware_priv *fw_priv;
> +	struct firmware_buf *buf;
>  	struct device *f_dev;
>  
> -	fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
> +	fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL);
>  	if (!fw_priv) {
>  		dev_err(device, "%s: kmalloc failed\n", __func__);
> -		return ERR_PTR(-ENOMEM);
> +		fw_priv = ERR_PTR(-ENOMEM);
> +		goto exit;
>  	}
>  
> -	fw_priv->fw = firmware;
> +	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_KERNEL);
> +	if (!buf) {
> +		dev_err(device, "%s: kmalloc failed\n", __func__);
> +		kfree(fw_priv);
> +		fw_priv = ERR_PTR(-ENOMEM);
> +		goto exit;
> +	}
> +
> +	buf->fw = firmware;
> +	fw_priv->buf = buf;
>  	fw_priv->nowait = nowait;
> -	strcpy(fw_priv->fw_id, fw_name);
> -	init_completion(&fw_priv->completion);
>  	setup_timer(&fw_priv->timeout,
>  		    firmware_class_timeout, (u_long) fw_priv);
> +	strcpy(buf->fw_id, fw_name);
> +	init_completion(&buf->completion);
>  
>  	f_dev = &fw_priv->dev;
>  
> @@ -455,7 +481,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>  	dev_set_name(f_dev, "%s", fw_name);
>  	f_dev->parent = device;
>  	f_dev->class = &firmware_class;
> -
> +exit:

Ditto: please indent label names one position to the right.

[ … ]

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf

[Ming Lei]

On Wed, Jul 25, 2012 at 9:59 PM, Borislav Petkov <bp@amd64.org> wrote:

> This is subtle: the caller of fw_free_buf might forget to assign NULL to
> the buf ptr.

Who is the caller? Since it is always called inside firmware loader, we should
make sure that.

> Why not pass struct firmware_priv *fw_priv to the function instead and ...

No, it shouldn't. The lifetime of fw_priv is just same with request_firmware or
its work_func pair, but firmware_buf may live much longer than fw_priv. You
will see that fw_free_buf is the release function of kref in firmware_buf.

>
>> +
>> +     for (i = 0; i < buf->nr_pages; i++)
>> +             __free_page(buf->pages[i]);
>> +     kfree(buf->pages);
>
> assign NULL to the ptr as a last step, when all is done:
>
>         fw_priv->buf = NULL;
>
> This way you're making sure ->buf is NULL after all pages are freed and
> your check above is always correct.

It has been done in _request_firmware_load


>> -             kunmap(fw_priv->pages[page_nr]);
>> +             kunmap(buf->pages[page_nr]);
>>               buffer += page_cnt;
>>               offset += page_cnt;
>>               count -= page_cnt;
>> @@ -320,12 +334,13 @@ out:
>
> While you're at it, you can indent this "out:" label one space to the
> right so that the diff can pick up the function name in the hunk tag
> above instead of the label.

Suppose you are right, it shouldn't be done in this patch since this patch
just converts to firmware_buf.


Thanks,
--
Ming Lei

Re: [RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf

[Borislav Petkov]

On Thu, Jul 26, 2012 at 10:51:55AM +0800, Ming Lei wrote:
> On Wed, Jul 25, 2012 at 9:59 PM, Borislav Petkov <bp@amd64.org> wrote:
> 
> > This is subtle: the caller of fw_free_buf might forget to assign NULL to
> > the buf ptr.
> 
> Who is the caller? Since it is always called inside firmware loader, we should
> make sure that.
> 
> > Why not pass struct firmware_priv *fw_priv to the function instead and ...
> 
> No, it shouldn't. The lifetime of fw_priv is just same with request_firmware or
> its work_func pair, but firmware_buf may live much longer than fw_priv. You
> will see that fw_free_buf is the release function of kref in firmware_buf.

Actually, this is all moot since you're changing all this in later
patches. I'm staring at the code after patch 10/13 and the check is
gone.

So nevermind.

[ … ]

> Suppose you are right, it shouldn't be done in this patch since this
> patch just converts to firmware_buf.

Wherever you find its suitable.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Ming Lei]

This patch always let firmware_buf own the pages buffer allocated
inside firmware_data_write, also add all instances of firmware_buf
into the firmware cache global list. Also introduce one private field
in 'struct firmware', so release_firmware will see the instance of
firmware_buf associated with one firmware instance, then just 'free'
the instance of firmware_buf.

The firmware_buf instance represents one pages buffer for one
firmware image, so lots of firmware loading requests can share
the same firmware_buf instance if they request the same firmware
image file.

This patch will make introducing cache_firmware/uncache_firmware
easily.

In fact, the patch improves request_formware/release_firmware:

        - only request userspace to write firmware image once if
	several devices share one same firmware image and its drivers
	call request_firmware concurrently.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |  222 ++++++++++++++++++++++++++++-------------
 include/linux/firmware.h      |    3 +
 2 files changed, 157 insertions(+), 68 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 986d9df..225898e 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -21,6 +21,7 @@
 #include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/list.h>
 
 MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
@@ -85,13 +86,18 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
 }
 
-/* fw_lock could be moved to 'struct firmware_priv' but since it is just
- * guarding for corner cases a global lock should be OK */
-static DEFINE_MUTEX(fw_lock);
+struct firmware_cache {
+
+	/* firmware_buf instance will be added into the below list */
+	spinlock_t lock;
+	struct list_head head;
+};
 
 struct firmware_buf {
+	struct kref ref;
+	struct list_head list;
 	struct completion completion;
-	struct firmware *fw;
+	struct firmware_cache *fwc;
 	unsigned long status;
 	void *data;
 	size_t size;
@@ -106,8 +112,93 @@ struct firmware_priv {
 	bool nowait;
 	struct device dev;
 	struct firmware_buf *buf;
+	struct firmware *fw;
 };
 
+#define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
+
+/* fw_lock could be moved to 'struct firmware_priv' but since it is just
+ * guarding for corner cases a global lock should be OK */
+static DEFINE_MUTEX(fw_lock);
+
+static struct firmware_cache fw_cache;
+
+static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
+		struct firmware_cache *fwc)
+{
+	struct firmware_buf *buf;
+
+	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1 , GFP_ATOMIC);
+
+	if (!buf)
+		return buf;
+
+	kref_init(&buf->ref);
+	strcpy(buf->fw_id, fw_name);
+	buf->fwc = fwc;
+	init_completion(&buf->completion);
+
+	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
+
+	return buf;
+}
+
+static int fw_lookup_and_alloate_buf(const char *fw_name,
+		struct firmware_cache *fwc,
+		struct firmware_buf **buf)
+{
+	struct firmware_buf *tmp;
+
+	spin_lock(&fwc->lock);
+	list_for_each_entry(tmp, &fwc->head, list)
+		if (!strcmp(tmp->fw_id, fw_name)) {
+			kref_get(&tmp->ref);
+			spin_unlock(&fwc->lock);
+			*buf = tmp;
+			return 1;
+		}
+
+	tmp = __allocate_fw_buf(fw_name, fwc);
+	if (tmp)
+		list_add(&tmp->list, &fwc->head);
+	spin_unlock(&fwc->lock);
+
+	*buf = tmp;
+
+	return tmp ? 0 : -1;
+}
+
+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 nr_pages=%d\n",
+			__func__, buf->fw_id, buf, buf->nr_pages);
+
+	spin_lock(&fwc->lock);
+	list_del(&buf->list);
+	spin_unlock(&fwc->lock);
+
+	vunmap(buf->data);
+	for (i = 0; i < buf->nr_pages; i++)
+		__free_page(buf->pages[i]);
+	kfree(buf->pages);
+	kfree(buf);
+}
+
+static void fw_free_buf(struct firmware_buf *buf)
+{
+	kref_put(&buf->ref, __fw_free_buf);
+}
+
+static void fw_cache_init(void)
+{
+	spin_lock_init(&fw_cache.lock);
+	INIT_LIST_HEAD(&fw_cache.head);
+}
+
 static struct firmware_priv *to_firmware_priv(struct device *dev)
 {
 	return container_of(dev, struct firmware_priv, dev);
@@ -118,7 +209,7 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	struct firmware_buf *buf = fw_priv->buf;
 
 	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete(&buf->completion);
+	complete_all(&buf->completion);
 }
 
 static ssize_t firmware_timeout_show(struct class *class,
@@ -158,23 +249,10 @@ static struct class_attribute firmware_class_attrs[] = {
 	__ATTR_NULL
 };
 
-static void fw_free_buf(struct firmware_buf *buf)
-{
-	int i;
-
-	if (!buf)
-		return;
-
-	for (i = 0; i < buf->nr_pages; i++)
-		__free_page(buf->pages[i]);
-	kfree(buf->pages);
-}
-
 static void fw_dev_release(struct device *dev)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
-	kfree(fw_priv->buf);
 	kfree(fw_priv);
 
 	module_put(THIS_MODULE);
@@ -213,13 +291,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 /* firmware holds the ownership of pages */
 static void firmware_free_data(const struct firmware *fw)
 {
-	int i;
-	vunmap(fw->data);
-	if (fw->pages) {
-		for (i = 0; i < PFN_UP(fw->size); i++)
-			__free_page(fw->pages[i]);
-		kfree(fw->pages);
-	}
+	WARN_ON(!fw->priv);
+	fw_free_buf(fw->priv);
 }
 
 /* Some architectures don't have PAGE_KERNEL_RO */
@@ -270,7 +343,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
 			set_bit(FW_STATUS_DONE, &fw_buf->status);
 			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-			complete(&fw_buf->completion);
+			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -446,10 +519,10 @@ static void firmware_class_timeout(u_long data)
 
 static struct firmware_priv *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-		   struct device *device, bool uevent, bool nowait)
+			struct device *device, bool uevent, bool nowait)
+
 {
 	struct firmware_priv *fw_priv;
-	struct firmware_buf *buf;
 	struct device *f_dev;
 
 	fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL);
@@ -459,21 +532,10 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 		goto exit;
 	}
 
-	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_KERNEL);
-	if (!buf) {
-		dev_err(device, "%s: kmalloc failed\n", __func__);
-		kfree(fw_priv);
-		fw_priv = ERR_PTR(-ENOMEM);
-		goto exit;
-	}
-
-	buf->fw = firmware;
-	fw_priv->buf = buf;
 	fw_priv->nowait = nowait;
+	fw_priv->fw = firmware;
 	setup_timer(&fw_priv->timeout,
 		    firmware_class_timeout, (u_long) fw_priv);
-	strcpy(buf->fw_id, fw_name);
-	init_completion(&buf->completion);
 
 	f_dev = &fw_priv->dev;
 
@@ -485,12 +547,31 @@ exit:
 	return fw_priv;
 }
 
+/* store the pages buffer info firmware from buf */
+static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
+{
+	buf->data = vmap(buf->pages, buf->nr_pages,
+				0, PAGE_KERNEL_RO);
+	fw->data = buf->data;
+	fw->pages = buf->pages;
+	fw->size = buf->size;
+	fw->priv = buf;
+}
+
+static void _request_firmware_cleanup(const 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)
 {
 	struct firmware *firmware;
-	struct firmware_priv *fw_priv;
+	struct firmware_priv *fw_priv = NULL;
+	struct firmware_buf *buf;
+	int ret;
 
 	if (!firmware_p)
 		return ERR_PTR(-EINVAL);
@@ -507,32 +588,40 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
 		return NULL;
 	}
 
-	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
-	if (IS_ERR(fw_priv)) {
-		release_firmware(firmware);
-		*firmware_p = NULL;
-	}
-	return fw_priv;
-}
+	ret = fw_lookup_and_alloate_buf(name, &fw_cache, &buf);
 
-static void _request_firmware_cleanup(const struct firmware **firmware_p)
-{
-	release_firmware(*firmware_p);
-	*firmware_p = NULL;
-}
+	if (!ret)
+		fw_priv = fw_create_instance(firmware, name, device,
+				uevent, nowait);
 
-/* transfer the ownership of pages to firmware */
-static void fw_set_page_data(struct firmware_buf *buf)
-{
-	struct firmware *fw = buf->fw;
+	if (IS_ERR(fw_priv) || ret == -1) {
+		kfree(firmware);
+		*firmware_p = NULL;
+		return ERR_PTR(-ENOMEM);
+	} else if (fw_priv) {
+		fw_priv->buf = buf;
+		return fw_priv;
+	}
 
-	buf->data = vmap(buf->pages, buf->nr_pages,
-				0, PAGE_KERNEL_RO);
-	fw->data = buf->data;
-	fw->pages = buf->pages;
-	fw->size = buf->size;
+	/* 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);
+		_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;
+	}
+	mutex_unlock(&fw_lock);
+	wait_for_completion(&buf->completion);
+	goto check_status;
 
-	WARN_ON(PFN_UP(fw->size) != buf->nr_pages);
+exit:
+	mutex_unlock(&fw_lock);
+	return fw_priv;
 }
 
 static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
@@ -583,11 +672,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
 		retval = -ENOENT;
 
-	/* transfer pages ownership at the last minute */
-	if (!retval)
-		fw_set_page_data(buf);
-	else
-		fw_free_buf(buf); /* free untransfered pages buffer */
+	fw_set_page_data(buf, fw_priv->fw);
 
 	fw_priv->buf = NULL;
 	mutex_unlock(&fw_lock);
@@ -750,6 +835,7 @@ request_firmware_nowait(
 
 static int __init firmware_class_init(void)
 {
+	fw_cache_init();
 	return class_register(&firmware_class);
 }
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 1e7c011..e85b771 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -12,6 +12,9 @@ struct firmware {
 	size_t size;
 	const u8 *data;
 	struct page **pages;
+
+	/* firmware loader private fields */
+	void *priv;
 };
 
 struct module;
-- 
1.7.9.5

Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Stephen Boyd]

Mostly trivia.

On 7/24/2012 10:00 AM, Ming Lei wrote:
> +
> +static int fw_lookup_and_alloate_buf(const char *fw_name,

allocate?

> +		struct firmware_cache *fwc,
> +		struct firmware_buf **buf)
> +{
> +	struct firmware_buf *tmp;
> +
> +	spin_lock(&fwc->lock);
> +	list_for_each_entry(tmp, &fwc->head, list)
> +		if (!strcmp(tmp->fw_id, fw_name)) {
> +			kref_get(&tmp->ref);
> +			spin_unlock(&fwc->lock);
> +			*buf = tmp;
> +			return 1;
> +		}
> +
> +	tmp = __allocate_fw_buf(fw_name, fwc);
> +	if (tmp)
> +		list_add(&tmp->list, &fwc->head);
> +	spin_unlock(&fwc->lock);
> +
> +	*buf = tmp;
> +
> +	return tmp ? 0 : -1;

-ENOMEM instead of -1?

> +
> +static void fw_cache_init(void)

__init?

> +{
> +	spin_lock_init(&fw_cache.lock);
> +	INIT_LIST_HEAD(&fw_cache.head);
> +}
> +
[...]
> uevent, nowait);
>  
> -/* transfer the ownership of pages to firmware */
> -static void fw_set_page_data(struct firmware_buf *buf)
> -{
> -	struct firmware *fw = buf->fw;
> +	if (IS_ERR(fw_priv) || ret == -1) {
> +		kfree(firmware);
> +		*firmware_p = NULL;
> +		return ERR_PTR(-ENOMEM);

This would have to check for < 0 and return ERR_PTR(ret) instead.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:06AM +0800, Ming Lei wrote:
> This patch always let firmware_buf own the pages buffer allocated
> inside firmware_data_write, also add all instances of firmware_buf
> into the firmware cache global list. Also introduce one private field
> in 'struct firmware', so release_firmware will see the instance of
> firmware_buf associated with one firmware instance, then just 'free'
> the instance of firmware_buf.
> 
> The firmware_buf instance represents one pages buffer for one
> firmware image, so lots of firmware loading requests can share
> the same firmware_buf instance if they request the same firmware
> image file.
> 
> This patch will make introducing cache_firmware/uncache_firmware
> easily.
> 
> In fact, the patch improves request_formware/release_firmware:
> 
>         - only request userspace to write firmware image once if
> 	several devices share one same firmware image and its drivers
> 	call request_firmware concurrently.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |  222 ++++++++++++++++++++++++++++-------------
>  include/linux/firmware.h      |    3 +
>  2 files changed, 157 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 986d9df..225898e 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -21,6 +21,7 @@
>  #include <linux/firmware.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> +#include <linux/list.h>
>  
>  MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -85,13 +86,18 @@ static inline long firmware_loading_timeout(void)
>  	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
>  }
>  
> -/* fw_lock could be moved to 'struct firmware_priv' but since it is just
> - * guarding for corner cases a global lock should be OK */
> -static DEFINE_MUTEX(fw_lock);
> +struct firmware_cache {
> +
> +	/* firmware_buf instance will be added into the below list */
> +	spinlock_t lock;
> +	struct list_head head;
> +};
>  
>  struct firmware_buf {
> +	struct kref ref;
> +	struct list_head list;
>  	struct completion completion;
> -	struct firmware *fw;
> +	struct firmware_cache *fwc;
>  	unsigned long status;
>  	void *data;
>  	size_t size;
> @@ -106,8 +112,93 @@ struct firmware_priv {
>  	bool nowait;
>  	struct device dev;
>  	struct firmware_buf *buf;
> +	struct firmware *fw;
>  };
>  
> +#define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
> +
> +/* fw_lock could be moved to 'struct firmware_priv' but since it is just
> + * guarding for corner cases a global lock should be OK */
> +static DEFINE_MUTEX(fw_lock);
> +
> +static struct firmware_cache fw_cache;
> +
> +static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
> +		struct firmware_cache *fwc)

Please indent like so:

static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
					      struct firmware_cache *fwc)

for better readability.

> +{
> +	struct firmware_buf *buf;
> +
> +	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1 , GFP_ATOMIC);
> +
> +	if (!buf)
> +		return buf;
> +
> +	kref_init(&buf->ref);
> +	strcpy(buf->fw_id, fw_name);
> +	buf->fwc = fwc;
> +	init_completion(&buf->completion);
> +
> +	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
> +
> +	return buf;
> +}
> +
> +static int fw_lookup_and_alloate_buf(const char *fw_name,
> +		struct firmware_cache *fwc,
> +		struct firmware_buf **buf)

Ditto.

> +{
> +	struct firmware_buf *tmp;
> +
> +	spin_lock(&fwc->lock);
> +	list_for_each_entry(tmp, &fwc->head, list)
> +		if (!strcmp(tmp->fw_id, fw_name)) {
> +			kref_get(&tmp->ref);
> +			spin_unlock(&fwc->lock);
> +			*buf = tmp;
> +			return 1;
> +		}
> +
> +	tmp = __allocate_fw_buf(fw_name, fwc);
> +	if (tmp)
> +		list_add(&tmp->list, &fwc->head);
> +	spin_unlock(&fwc->lock);
> +
> +	*buf = tmp;
> +
> +	return tmp ? 0 : -1;
> +}
> +
> +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 nr_pages=%d\n",
> +			__func__, buf->fw_id, buf, buf->nr_pages);

Arg alignment:

	pr_debug("%s: fw-%s buf=%p nr_pages=%d\n",
		 __func__, buf->fw_id, buf, buf->nr_pages);

> +	spin_lock(&fwc->lock);
> +	list_del(&buf->list);
> +	spin_unlock(&fwc->lock);
> +
> +	vunmap(buf->data);
> +	for (i = 0; i < buf->nr_pages; i++)
> +		__free_page(buf->pages[i]);
> +	kfree(buf->pages);
> +	kfree(buf);
> +}
> +
> +static void fw_free_buf(struct firmware_buf *buf)
> +{
> +	kref_put(&buf->ref, __fw_free_buf);
> +}
> +
> +static void fw_cache_init(void)
> +{
> +	spin_lock_init(&fw_cache.lock);
> +	INIT_LIST_HEAD(&fw_cache.head);
> +}
> +
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
>  {
>  	return container_of(dev, struct firmware_priv, dev);
> @@ -118,7 +209,7 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  	struct firmware_buf *buf = fw_priv->buf;
>  
>  	set_bit(FW_STATUS_ABORT, &buf->status);
> -	complete(&buf->completion);
> +	complete_all(&buf->completion);
>  }
>  
>  static ssize_t firmware_timeout_show(struct class *class,
> @@ -158,23 +249,10 @@ static struct class_attribute firmware_class_attrs[] = {
>  	__ATTR_NULL
>  };
>  
> -static void fw_free_buf(struct firmware_buf *buf)
> -{
> -	int i;
> -
> -	if (!buf)
> -		return;
> -
> -	for (i = 0; i < buf->nr_pages; i++)
> -		__free_page(buf->pages[i]);
> -	kfree(buf->pages);
> -}
> -
>  static void fw_dev_release(struct device *dev)
>  {
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  
> -	kfree(fw_priv->buf);
>  	kfree(fw_priv);
>  
>  	module_put(THIS_MODULE);
> @@ -213,13 +291,8 @@ static ssize_t firmware_loading_show(struct device *dev,
>  /* firmware holds the ownership of pages */
>  static void firmware_free_data(const struct firmware *fw)
>  {
> -	int i;
> -	vunmap(fw->data);
> -	if (fw->pages) {
> -		for (i = 0; i < PFN_UP(fw->size); i++)
> -			__free_page(fw->pages[i]);
> -		kfree(fw->pages);
> -	}
> +	WARN_ON(!fw->priv);
> +	fw_free_buf(fw->priv);

Why the WARN_ON?

Maybe rather:

	if (fw->priv)
		fw_free_buf(fw->priv);

?

>  /* Some architectures don't have PAGE_KERNEL_RO */
> @@ -270,7 +343,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
>  			set_bit(FW_STATUS_DONE, &fw_buf->status);
>  			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> -			complete(&fw_buf->completion);
> +			complete_all(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -446,10 +519,10 @@ static void firmware_class_timeout(u_long data)
>  
>  static struct firmware_priv *
>  fw_create_instance(struct firmware *firmware, const char *fw_name,
> -		   struct device *device, bool uevent, bool nowait)
> +			struct device *device, bool uevent, bool nowait)
> +

Wrong alignment and superfluous newline.

>  {
>  	struct firmware_priv *fw_priv;
> -	struct firmware_buf *buf;
>  	struct device *f_dev;
>  
>  	fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL);
> @@ -459,21 +532,10 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>  		goto exit;
>  	}
>  
> -	buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_KERNEL);
> -	if (!buf) {
> -		dev_err(device, "%s: kmalloc failed\n", __func__);
> -		kfree(fw_priv);
> -		fw_priv = ERR_PTR(-ENOMEM);
> -		goto exit;
> -	}
> -
> -	buf->fw = firmware;
> -	fw_priv->buf = buf;
>  	fw_priv->nowait = nowait;
> +	fw_priv->fw = firmware;
>  	setup_timer(&fw_priv->timeout,
>  		    firmware_class_timeout, (u_long) fw_priv);
> -	strcpy(buf->fw_id, fw_name);
> -	init_completion(&buf->completion);
>  
>  	f_dev = &fw_priv->dev;
>  
> @@ -485,12 +547,31 @@ exit:
>  	return fw_priv;
>  }
>  
> +/* store the pages buffer info firmware from buf */
> +static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
> +{
> +	buf->data = vmap(buf->pages, buf->nr_pages,
> +				0, PAGE_KERNEL_RO);
> +	fw->data = buf->data;
> +	fw->pages = buf->pages;
> +	fw->size = buf->size;
> +	fw->priv = buf;
> +}
> +
> +static void _request_firmware_cleanup(const 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)
>  {
>  	struct firmware *firmware;
> -	struct firmware_priv *fw_priv;
> +	struct firmware_priv *fw_priv = NULL;
> +	struct firmware_buf *buf;
> +	int ret;
>  
>  	if (!firmware_p)
>  		return ERR_PTR(-EINVAL);
> @@ -507,32 +588,40 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
>  		return NULL;
>  	}
>  
> -	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
> -	if (IS_ERR(fw_priv)) {
> -		release_firmware(firmware);
> -		*firmware_p = NULL;
> -	}
> -	return fw_priv;
> -}
> +	ret = fw_lookup_and_alloate_buf(name, &fw_cache, &buf);
>  
> -static void _request_firmware_cleanup(const struct firmware **firmware_p)
> -{
> -	release_firmware(*firmware_p);
> -	*firmware_p = NULL;
> -}

Superfluous newline here.

> +	if (!ret)
> +		fw_priv = fw_create_instance(firmware, name, device,
> +				uevent, nowait);
>  
> -/* transfer the ownership of pages to firmware */
> -static void fw_set_page_data(struct firmware_buf *buf)
> -{
> -	struct firmware *fw = buf->fw;
> +	if (IS_ERR(fw_priv) || ret == -1) {
> +		kfree(firmware);
> +		*firmware_p = NULL;
> +		return ERR_PTR(-ENOMEM);
> +	} else if (fw_priv) {
> +		fw_priv->buf = buf;
> +		return fw_priv;
> +	}
>  
> -	buf->data = vmap(buf->pages, buf->nr_pages,
> -				0, PAGE_KERNEL_RO);
> -	fw->data = buf->data;
> -	fw->pages = buf->pages;
> -	fw->size = buf->size;
> +	/* share the cached buf, which is inprogessing or completed */

					in progress

> +check_status:

One space in front of the label name.

> +	mutex_lock(&fw_lock);
> +	if (test_bit(FW_STATUS_ABORT, &buf->status)) {
> +		fw_priv = ERR_PTR(-ENOENT);
> +		_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;
> +	}
> +	mutex_unlock(&fw_lock);
> +	wait_for_completion(&buf->completion);
> +	goto check_status;

Better yet, this is a label with a reverse-jump to it. This can probably
be simplified with a while loop:

	while (true) {
		if (test_bit(... ) {
			...;
			break;
		} else if (test_bit(... ) {
			...;
			break;
		}
		mutex_unlock(...);
		wait_for_completion(...);
	}

and this way you don't need the exit label either.

>  
> -	WARN_ON(PFN_UP(fw->size) != buf->nr_pages);
> +exit:
> +	mutex_unlock(&fw_lock);
> +	return fw_priv;
>  }

[ … ]

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Ming Lei]

On Wed, Jul 25, 2012 at 10:37 PM, Borislav Petkov <bp@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:06AM +0800, Ming Lei wrote:

>> @@ -213,13 +291,8 @@ static ssize_t firmware_loading_show(struct device *dev,
>>  /* firmware holds the ownership of pages */
>>  static void firmware_free_data(const struct firmware *fw)
>>  {
>> -     int i;
>> -     vunmap(fw->data);
>> -     if (fw->pages) {
>> -             for (i = 0; i < PFN_UP(fw->size); i++)
>> -                     __free_page(fw->pages[i]);
>> -             kfree(fw->pages);
>> -     }
>> +     WARN_ON(!fw->priv);
>> +     fw_free_buf(fw->priv);
>
> Why the WARN_ON?

Because the 'struct firmware' instance is exposed to drivers, it is
better to warn if some drivers clear it during request&release.

Thanks,
--
Ming Lei

Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:06AM +0800, Ming Lei wrote:
> @@ -750,6 +835,7 @@ request_firmware_nowait(

while you're here, can you fix the arg alignment of this
request_firmware_nowait - it looks awful right now:

int
request_firmware_nowait(
        struct module *module, bool uevent,
        const char *name, struct device *device, gfp_t gfp, void *context,
        void (*cont)(const struct firmware *fw, void *context))
{

and it could like, say, this:

int request_firmware_nowait(struct module *module, bool uevent, const char *name,
			    struct device *device, gfp_t gfp, void *context,
			    void (*cont)(const struct firmware *fw, void *context))
{

or something even more readable.

>  static int __init firmware_class_init(void)
>  {
> +	fw_cache_init();
>  	return class_register(&firmware_class);
>  }
>  
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 1e7c011..e85b771 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -12,6 +12,9 @@ struct firmware {
>  	size_t size;
>  	const u8 *data;
>  	struct page **pages;
> +
> +	/* firmware loader private fields */
> +	void *priv;
>  };
>  
>  struct module;

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:06AM +0800, Ming Lei wrote:
> This patch always let firmware_buf own the pages buffer allocated
> inside firmware_data_write, also add all instances of firmware_buf
> into the firmware cache global list. Also introduce one private field
> in 'struct firmware', so release_firmware will see the instance of
> firmware_buf associated with one firmware instance, then just 'free'
> the instance of firmware_buf.
> 
> The firmware_buf instance represents one pages buffer for one
> firmware image, so lots of firmware loading requests can share
> the same firmware_buf instance if they request the same firmware
> image file.
> 
> This patch will make introducing cache_firmware/uncache_firmware
> easily.
> 
> In fact, the patch improves request_formware/release_firmware:
> 
>         - only request userspace to write firmware image once if
> 	several devices share one same firmware image and its drivers
> 	call request_firmware concurrently.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |  222 ++++++++++++++++++++++++++++-------------
>  include/linux/firmware.h      |    3 +
>  2 files changed, 157 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 986d9df..225898e 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -21,6 +21,7 @@
>  #include <linux/firmware.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> +#include <linux/list.h>
>  
>  MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -85,13 +86,18 @@ static inline long firmware_loading_timeout(void)
>  	return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
>  }
>  
> -/* fw_lock could be moved to 'struct firmware_priv' but since it is just
> - * guarding for corner cases a global lock should be OK */
> -static DEFINE_MUTEX(fw_lock);
> +struct firmware_cache {
> +

Stray newline. Sorry I missed it the first time.

> +	/* firmware_buf instance will be added into the below list */
> +	spinlock_t lock;
> +	struct list_head head;
> +};

[ … ]

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[RFC PATCH 07/13] driver core: firmware loader: introduce cache_firmware and uncache_firmware

[Ming Lei]

This patches introduce two kernel APIs of cache_firmware and
uncache_firmware, both of which take the firmware file name
as the only parameter.

So any drivers can call cache_firmware to cache the specified
firmware file into kernel memory, and can use the cached firmware
in situations which can't request firmware from user space.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   79 +++++++++++++++++++++++++++++++++++++++++
 include/linux/firmware.h      |   12 +++++++
 2 files changed, 91 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 225898e..674cb11 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -168,6 +168,22 @@ static int fw_lookup_and_alloate_buf(const char *fw_name,
 	return tmp ? 0 : -1;
 }
 
+static struct firmware_buf *fw_lookup_buf(const char *fw_name)
+{
+	struct firmware_buf *tmp;
+	struct firmware_cache *fwc = &fw_cache;
+
+	spin_lock(&fwc->lock);
+	list_for_each_entry(tmp, &fwc->head, list)
+		if (!strcmp(tmp->fw_id, fw_name)) {
+			spin_unlock(&fwc->lock);
+			return tmp;
+		}
+	spin_unlock(&fwc->lock);
+
+	return NULL;
+}
+
 static void __fw_free_buf(struct kref *ref)
 {
 	struct firmware_buf *buf = to_fwbuf(ref);
@@ -833,6 +849,67 @@ request_firmware_nowait(
 	return 0;
 }
 
+/**
+ * cache_firmware - cache one firmware image in kernel memory space
+ * @fw_name: the firmware image name
+ *
+ * Cache firmware in kernel memory so that drivers can use the firmware
+ * when system isn't ready for drivers to request firmware image from
+ * userspace. Once returns successfully, driver can use request_firmware*
+ * to get the cached firmware without any interacting with user space
+ *
+ * Return 0 if the firmware image has been cached successfully
+ * Return !0 if it is not successfully
+ *
+ */
+int cache_firmware(const char *fw_name)
+{
+	int ret;
+	const struct firmware *fw;
+
+	pr_debug("%s: %s\n", __func__, fw_name);
+
+	ret = request_firmware(&fw, fw_name, NULL);
+
+	if (!ret)
+		kfree(fw);
+
+	pr_debug("%s: %s ret=%d\n", __func__, fw_name, ret);
+
+	return ret;
+}
+
+/**
+ * uncache_firmware - remove one cached firmware image
+ *
+ * @fw_name: the firmware image name
+ *
+ * Uncache one firmware image which has been cached successfully
+ * before.
+ *
+ * Return 0 if the firmware cache has been removed successfully
+ * Return !0 if it is not successfully
+ *
+ */
+int uncache_firmware(const char *fw_name)
+{
+	struct firmware_buf *buf;
+	struct firmware fw;
+
+	pr_debug("%s: %s\n", __func__, fw_name);
+
+	if (fw_get_builtin_firmware(&fw, fw_name))
+		return 0;
+
+	buf = fw_lookup_buf(fw_name);
+	if (buf) {
+		fw_free_buf(buf);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int __init firmware_class_init(void)
 {
 	fw_cache_init();
@@ -850,3 +927,5 @@ module_exit(firmware_class_exit);
 EXPORT_SYMBOL(release_firmware);
 EXPORT_SYMBOL(request_firmware);
 EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL_GPL(cache_firmware);
+EXPORT_SYMBOL_GPL(uncache_firmware);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e85b771..1bfa202 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+int cache_firmware(const char *name);
+int uncache_firmware(const char *name);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -65,6 +67,16 @@ static inline int request_firmware_nowait(
 static inline void release_firmware(const struct firmware *fw)
 {
 }
+
+int cache_firmware(const char *name)
+{
+	return -ENOENT;
+}
+
+int uncache_firmware(const char *name)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif
-- 
1.7.9.5

Re: [RFC PATCH 07/13] driver core: firmware loader: introduce cache_firmware and uncache_firmware

[Stephen Boyd]

On 7/24/2012 10:00 AM, Ming Lei wrote:
>
> +
> +int cache_firmware(const char *name)
> +{
> +	return -ENOENT;
> +}
> +
> +int uncache_firmware(const char *name)
> +{
> +	return -EINVAL;
> +}

These stubs need to be static inline to avoid compiler warnings.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC PATCH 07/13] driver core: firmware loader: introduce cache_firmware and uncache_firmware

[Ming Lei]

On Wed, Jul 25, 2012 at 3:54 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 7/24/2012 10:00 AM, Ming Lei wrote:
>>
>> +
>> +int cache_firmware(const char *name)
>> +{
>> +     return -ENOENT;
>> +}
>> +
>> +int uncache_firmware(const char *name)
>> +{
>> +     return -EINVAL;
>> +}
>
> These stubs need to be static inline to avoid compiler warnings.

Good catch, will fix it in v1.

Thanks,
--
Ming Lei

Re: [RFC PATCH 07/13] driver core: firmware loader: introduce cache_firmware and uncache_firmware

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:07AM +0800, Ming Lei wrote:
> This patches introduce two kernel APIs of cache_firmware and
> uncache_firmware, both of which take the firmware file name
> as the only parameter.
> 
> So any drivers can call cache_firmware to cache the specified
> firmware file into kernel memory, and can use the cached firmware
> in situations which can't request firmware from user space.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |   79 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware.h      |   12 +++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 225898e..674cb11 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -168,6 +168,22 @@ static int fw_lookup_and_alloate_buf(const char *fw_name,
>  	return tmp ? 0 : -1;
>  }
>  
> +static struct firmware_buf *fw_lookup_buf(const char *fw_name)
> +{
> +	struct firmware_buf *tmp;
> +	struct firmware_cache *fwc = &fw_cache;
> +
> +	spin_lock(&fwc->lock);
> +	list_for_each_entry(tmp, &fwc->head, list)
> +		if (!strcmp(tmp->fw_id, fw_name)) {
> +			spin_unlock(&fwc->lock);
> +			return tmp;
> +		}
> +	spin_unlock(&fwc->lock);
> +
> +	return NULL;
> +}

You have similar functionality in fw_lookup_and_alloate_buf() above.
Can't you reuse it instead of defining a new function?

> +
>  static void __fw_free_buf(struct kref *ref)
>  {
>  	struct firmware_buf *buf = to_fwbuf(ref);
> @@ -833,6 +849,67 @@ request_firmware_nowait(
>  	return 0;
>  }
>  
> +/**
> + * cache_firmware - cache one firmware image in kernel memory space
> + * @fw_name: the firmware image name
> + *
> + * Cache firmware in kernel memory so that drivers can use the firmware

								s/the firmware/it/

> + * when system isn't ready for drivers to request firmware image from

				s/drivers/them/

> + * userspace. Once returns successfully, driver can use request_firmware*

		    it

> + * to get the cached firmware without any interacting with user space

							s/user space/userspace/

> + *
> + * Return 0 if the firmware image has been cached successfully
> + * Return !0 if it is not successfully

	Return !0 otherwise.

> + *
> + */
> +int cache_firmware(const char *fw_name)
> +{
> +	int ret;
> +	const struct firmware *fw;
> +
> +	pr_debug("%s: %s\n", __func__, fw_name);
> +
> +	ret = request_firmware(&fw, fw_name, NULL);
> +

stray newline

> +	if (!ret)
> +		kfree(fw);
> +
> +	pr_debug("%s: %s ret=%d\n", __func__, fw_name, ret);
> +
> +	return ret;
> +}
> +
> +/**
> + * uncache_firmware - remove one cached firmware image
> + *

stray newline

> + * @fw_name: the firmware image name
> + *
> + * Uncache one firmware image which has been cached successfully
> + * before.
> + *
> + * Return 0 if the firmware cache has been removed successfully
> + * Return !0 if it is not successfully

Return !0 otherwise

> + *
> + */
> +int uncache_firmware(const char *fw_name)
> +{
> +	struct firmware_buf *buf;
> +	struct firmware fw;
> +
> +	pr_debug("%s: %s\n", __func__, fw_name);
> +
> +	if (fw_get_builtin_firmware(&fw, fw_name))
> +		return 0;
> +
> +	buf = fw_lookup_buf(fw_name);
> +	if (buf) {
> +		fw_free_buf(buf);
> +		return 0;
> +	}
> +
> +	return -EINVAL;

[ … ]

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 07/13] driver core: firmware loader: introduce cache_firmware and uncache_firmware

[Ming Lei]

On Wed, Jul 25, 2012 at 11:52 PM, Borislav Petkov <bp@amd64.org> wrote:
>
> You have similar functionality in fw_lookup_and_alloate_buf() above.
> Can't you reuse it instead of defining a new function?

The problem is that the lock can't be released between lookup and allocate
inside fw_lookup_and_alloate_buf(). I will try to introduce __fw_lookup_buf
and reuse it in both fw_lookup_and_alloate_buf and fw_lookup_buf.

Also will fix the comments and code style you mentioned in -v1.


Thanks,
--
Ming Lei

[RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Ming Lei]

Callers of request_firmware* must hold the reference count of
@device, otherwise it is easy to trigger oops since the firmware
loader device is the child of @device.

This patch adds comments about the usage. In fact, most of drivers
call request_firmware* in its probe() or open(), so the constraint
should be reasonable and satisfied easily.

Also this patch holds the reference cound of @device before
schedule_work() in request_firmware_nowait() to avoid that
the @device dies after request_firmware_nowait returns and before
the work is scheduled.

Also request_firmware_nowait should be called in atomic context now,
so fix the obsolete comments.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 674cb11..540b2e1 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -717,6 +717,8 @@ err_put_dev:
  *      @name will be used as $FIRMWARE in the uevent environment and
  *      should be distinctive enough not to be confused with any other
  *      firmware image for this or any other device.
+ *
+ *	Caller must hold the reference count of @device.
  **/
 int
 request_firmware(const struct firmware **firmware_p, const char *name,
@@ -798,6 +800,7 @@ static void request_firmware_work_func(struct work_struct *work)
 
  out:
 	fw_work->cont(fw, fw_work->context);
+	put_device(fw_work->device);
 
 	module_put(fw_work->module);
 	kfree(fw_work);
@@ -816,9 +819,10 @@ static void request_firmware_work_func(struct work_struct *work)
  * @cont: function will be called asynchronously when the firmware
  *	request is over.
  *
+ *	Caller must hold the reference count of @device.
+ *
  *	Asynchronous variant of request_firmware() for user contexts where
- *	it is not possible to sleep for long time. It can't be called
- *	in atomic contexts.
+ *	it is not possible to sleep for long time.
  **/
 int
 request_firmware_nowait(
@@ -844,6 +848,7 @@ request_firmware_nowait(
 		return -EFAULT;
 	}
 
+	get_device(fw_work->device);
 	INIT_WORK(&fw_work->work, request_firmware_work_func);
 	schedule_work(&fw_work->work);
 	return 0;
-- 
1.7.9.5

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:08AM +0800, Ming Lei wrote:
> Callers of request_firmware* must hold the reference count of
> @device, otherwise it is easy to trigger oops since the firmware
> loader device is the child of @device.
> 
> This patch adds comments about the usage. In fact, most of drivers
> call request_firmware* in its probe() or open(), so the constraint
> should be reasonable and satisfied easily.
> 
> Also this patch holds the reference cound of @device before

					count

> schedule_work() in request_firmware_nowait() to avoid that
> the @device dies after request_firmware_nowait returns and before
> the work is scheduled.
> 
> Also request_firmware_nowait should be called in atomic context now,
> so fix the obsolete comments.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 674cb11..540b2e1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -717,6 +717,8 @@ err_put_dev:
>   *      @name will be used as $FIRMWARE in the uevent environment and
>   *      should be distinctive enough not to be confused with any other
>   *      firmware image for this or any other device.
> + *
> + *	Caller must hold the reference count of @device.
>   **/
>  int
>  request_firmware(const struct firmware **firmware_p, const char *name,
> @@ -798,6 +800,7 @@ static void request_firmware_work_func(struct work_struct *work)
>  
>   out:
>  	fw_work->cont(fw, fw_work->context);
> +	put_device(fw_work->device);
>  
>  	module_put(fw_work->module);
>  	kfree(fw_work);
> @@ -816,9 +819,10 @@ static void request_firmware_work_func(struct work_struct *work)
>   * @cont: function will be called asynchronously when the firmware
>   *	request is over.
>   *
> + *	Caller must hold the reference count of @device.
> + *
>   *	Asynchronous variant of request_firmware() for user contexts where
> - *	it is not possible to sleep for long time. It can't be called
> - *	in atomic contexts.
> + *	it is not possible to sleep for long time.

Let's state it explicitly:

	"it is not allowed to sleep for it is called in atomic context."

>   **/
>  int
>  request_firmware_nowait(
> @@ -844,6 +848,7 @@ request_firmware_nowait(
>  		return -EFAULT;
>  	}
>  
> +	get_device(fw_work->device);
>  	INIT_WORK(&fw_work->work, request_firmware_work_func);
>  	schedule_work(&fw_work->work);
>  	return 0;

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Ming Lei]

On Thu, Jul 26, 2012 at 12:04 AM, Borislav Petkov <bp@amd64.org> wrote:
>> Also this patch holds the reference cound of @device before
>
>                                         count

Good catch, will fix it in -v1.


>> - *   it is not possible to sleep for long time. It can't be called
>> - *   in atomic contexts.
>> + *   it is not possible to sleep for long time.
>
> Let's state it explicitly:
>
>         "it is not allowed to sleep for it is called in atomic context."

Looks you understand it incorrectly, the sentence is below

 *      Asynchronous variant of request_firmware() for user contexts where
 *      it is not possible to sleep for long time.

and maybe it should be changed to below:

 *      Asynchronous variant of request_firmware() for user contexts where
 *      it is not possible to sleep for long time or can't sleep at all.

Thanks,
--
Ming Lei

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Borislav Petkov]

On Thu, Jul 26, 2012 at 10:59:08AM +0800, Ming Lei wrote:
> On Thu, Jul 26, 2012 at 12:04 AM, Borislav Petkov <bp@amd64.org> wrote:
> >> Also this patch holds the reference cound of @device before
> >
> >                                         count
> 
> Good catch, will fix it in -v1.
> 
> 
> >> - *   it is not possible to sleep for long time. It can't be called
> >> - *   in atomic contexts.
> >> + *   it is not possible to sleep for long time.
> >
> > Let's state it explicitly:
> >
> >         "it is not allowed to sleep for it is called in atomic context."
> 
> Looks you understand it incorrectly, the sentence is below
> 
>  *      Asynchronous variant of request_firmware() for user contexts where
>  *      it is not possible to sleep for long time.
> 
> and maybe it should be changed to below:
> 
>  *      Asynchronous variant of request_firmware() for user contexts where
>  *      it is not possible to sleep for long time or can't sleep at all.

Ok, here's what I got from looking at the patch:

Your commit message says: "Also request_firmware_nowait should be called
in atomic context now, so fix the obsolete comments."

Atomic context in my book means you're not allowed to sleep at all.

But the comment says that it is possible to sleep a little. This is very
wrongly formulated AFAICT.

But, since request_firmware_nowait receives a GFP mask as one of its
arguments and some of its callers don't supply GFP_ATOMIC then this
has nothing to do with atomic contexts at all. Then, you should simply
explain in the comment why exactly callers aren't allowed to be sleeping
for a long time. And using adjectives like "long" or "short" is very
misleading in such explanations so please be more specific as to why the
callers shouldn't be sleeping for extended periods of time.

I hope I'm making sense here...

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Ming Lei]

On Thu, Jul 26, 2012 at 8:20 PM, Borislav Petkov <bp@amd64.org> wrote:
>
> Ok, here's what I got from looking at the patch:
>
> Your commit message says: "Also request_firmware_nowait should be called
> in atomic context now, so fix the obsolete comments."
>
> Atomic context in my book means you're not allowed to sleep at all.

In fact, I mean the function can be called in atomic context now, and
I know some time ago the function will create kthread to execute
the request_firmware, and atomic context is not allowed.

So I remove the obsolete comment.

>
> But the comment says that it is possible to sleep a little. This is very
> wrongly formulated AFAICT.

The function can be run in both contexts, and I don't see any words which
says the function will sleep.

>
> But, since request_firmware_nowait receives a GFP mask as one of its
> arguments and some of its callers don't supply GFP_ATOMIC then this
> has nothing to do with atomic contexts at all. Then, you should simply
> explain in the comment why exactly callers aren't allowed to be sleeping
> for a long time. And using adjectives like "long" or "short" is very
> misleading in such explanations so please be more specific as to why the

It is the original one, and I don't think it is wrong. Also it
shouldn't be covered
by this patch.

Maybe I shouldn't have fixed the comment in this patch.


Thanks,
--
Ming Lei

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Borislav Petkov]

On Thu, Jul 26, 2012 at 11:44:48PM +0800, Ming Lei wrote:
> On Thu, Jul 26, 2012 at 8:20 PM, Borislav Petkov <bp@amd64.org> wrote:
> >
> > Ok, here's what I got from looking at the patch:
> >
> > Your commit message says: "Also request_firmware_nowait should be called
> > in atomic context now, so fix the obsolete comments."
> >
> > Atomic context in my book means you're not allowed to sleep at all.
> 
> In fact, I mean the function can be called in atomic context now, and
> I know some time ago the function will create kthread to execute
> the request_firmware, and atomic context is not allowed.

Right, but when called with GFP_KERNEL mask, it can sleep, right?

> > But the comment says that it is possible to sleep a little. This is very
> > wrongly formulated AFAICT.
> 
> The function can be run in both contexts, and I don't see any words which
> says the function will sleep.

"
...
 *	Asynchronous variant of request_firmware() for user contexts where
 *	it is not possible to sleep for long time.
 **/
"

Not possible to sleep for a long time means the function still *can*
sleep... even for short time. For a certain definion of "short."

> > But, since request_firmware_nowait receives a GFP mask as one of its
> > arguments and some of its callers don't supply GFP_ATOMIC then this
> > has nothing to do with atomic contexts at all. Then, you should simply
> > explain in the comment why exactly callers aren't allowed to be sleeping
> > for a long time. And using adjectives like "long" or "short" is very
> > misleading in such explanations so please be more specific as to why the
> 
> It is the original one, and I don't think it is wrong. Also it
> shouldn't be covered
> by this patch.
> 
> Maybe I shouldn't have fixed the comment in this patch.

Why, simply fix the comment to adhere to what the function does. And
since it can sleep, maybe the easiest fix is to say simply that:
"function can sleep", right?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Ming Lei]

On Fri, Jul 27, 2012 at 1:46 AM, Borislav Petkov <bp@amd64.org> wrote:
> On Thu, Jul 26, 2012 at 11:44:48PM +0800, Ming Lei wrote:
>> On Thu, Jul 26, 2012 at 8:20 PM, Borislav Petkov <bp@amd64.org> wrote:
>> >
>> > Ok, here's what I got from looking at the patch:
>> >
>> > Your commit message says: "Also request_firmware_nowait should be called
>> > in atomic context now, so fix the obsolete comments."
>> >
>> > Atomic context in my book means you're not allowed to sleep at all.
>>
>> In fact, I mean the function can be called in atomic context now, and
>> I know some time ago the function will create kthread to execute
>> the request_firmware, and atomic context is not allowed.
>
> Right, but when called with GFP_KERNEL mask, it can sleep, right?

>
>> > But the comment says that it is possible to sleep a little. This is very
>> > wrongly formulated AFAICT.
>>
>> The function can be run in both contexts, and I don't see any words which
>> says the function will sleep.
>
> "
> ...
>  *      Asynchronous variant of request_firmware() for user contexts where
>  *      it is not possible to sleep for long time.
>  **/
> "
>
> Not possible to sleep for a long time means the function still *can*
> sleep... even for short time. For a certain definion of "short."

In fact, many drivers like to use it in its probe() because too long sleep
in probe will slow down kernel boot if driver is built in kernel. During
kernel boot, rootfs is not mounted and user space is not ready, request_firmware
will timeout to cause problem.

Also after introducing work in this function, it is allowed to be called in
atomic context if 'gfp' is passed as GFP_ATOMIC, so I removed the obsolete
comments.

>
>> > But, since request_firmware_nowait receives a GFP mask as one of its
>> > arguments and some of its callers don't supply GFP_ATOMIC then this
>> > has nothing to do with atomic contexts at all. Then, you should simply
>> > explain in the comment why exactly callers aren't allowed to be sleeping
>> > for a long time. And using adjectives like "long" or "short" is very
>> > misleading in such explanations so please be more specific as to why the
>>
>> It is the original one, and I don't think it is wrong. Also it
>> shouldn't be covered
>> by this patch.
>>
>> Maybe I shouldn't have fixed the comment in this patch.
>
> Why, simply fix the comment to adhere to what the function does. And
> since it can sleep, maybe the easiest fix is to say simply that:
> "function can sleep", right?

No, the comment above is misleading and not useless, and I think the below
is good:

 *      Asynchronous variant of request_firmware() for user contexts where
 *      it is not possible to sleep for long time or can't sleep at all, depends
 *      on the @gfp flag passed.

Anyway, the original part of 'It can't be called in atomic contexts.' is wrong
and should be removed.


Thanks,
--
Ming Lei

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Borislav Petkov]

On Fri, Jul 27, 2012 at 09:30:57AM +0800, Ming Lei wrote:
> No, the comment above is misleading and not useless, and I think the below
> is good:
> 
>  *      Asynchronous variant of request_firmware() for user contexts where
>  *      it is not possible to sleep for long time or can't sleep at all, depends

									depending

>  *      on the @gfp flag passed.
> 
> Anyway, the original part of 'It can't be called in atomic contexts.' is wrong
> and should be removed.

I still don't like too much the "not possible to sleep for long time"
expression.

Maybe change it to "should sleep for as small periods as possible since
it increases boot time of device drivers requesting firmware in their
->probe() methods."

This way you explain exactly why - this way people who don't know the
code will know exactly what the comments mean and what the intention
was.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 08/13] driver core: firmware loader: fix device lifetime

[Ming Lei]

On Fri, Jul 27, 2012 at 6:32 PM, Borislav Petkov <bp@amd64.org> wrote:

>
> I still don't like too much the "not possible to sleep for long time"
> expression.
>
> Maybe change it to "should sleep for as small periods as possible since
> it increases boot time of device drivers requesting firmware in their
> ->probe() methods."

Fairly enough, will add this kind of description in -v1, and I will introduce
one extra patch to fix this comment since it is nothing related with the
device lifetime fix and its comments.


Thanks,
--
Ming Lei

[RFC PATCH 09/13] driver core: firmware loader: store firmware name into devres list

[Ming Lei]

This patch will store firmware name into devres list of the device
which is requesting firmware loading, so that we can implement
auto cache firmware for devices in need.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   66 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 540b2e1..c181e6d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -115,6 +115,11 @@ struct firmware_priv {
 	struct firmware *fw;
 };
 
+struct fw_name_devm {
+	unsigned long magic;
+	char name[];
+};
+
 #define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
 
 /* fw_lock could be moved to 'struct firmware_priv' but since it is just
@@ -574,6 +579,56 @@ static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
 	fw->priv = buf;
 }
 
+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;
+}
+
 static void _request_firmware_cleanup(const struct firmware **firmware_p)
 {
 	release_firmware(*firmware_p);
@@ -690,6 +745,17 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	fw_set_page_data(buf, fw_priv->fw);
 
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * firmware for device.
+	 *
+	 * f_dev->parent may has been deleted already, but the problem
+	 * should be fixed in devres.
+	 *
+	 */
+	if (!retval && f_dev->parent)
+		fw_add_devm_name(f_dev->parent, buf->fw_id);
+
 	fw_priv->buf = NULL;
 	mutex_unlock(&fw_lock);
 
-- 
1.7.9.5

Re: [RFC PATCH 09/13] driver core: firmware loader: store firmware name into devres list

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:09AM +0800, Ming Lei wrote:
> This patch will store firmware name into devres list of the device
> which is requesting firmware loading, so that we can implement
> auto cache firmware for devices in need.

Stupid question: does this mean that once the firmware name is in the
devres list, it is being cached automatically and device drivers which
don't want that need to explicitly uncache it?

> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |   66 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 540b2e1..c181e6d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -115,6 +115,11 @@ struct firmware_priv {
>  	struct firmware *fw;
>  };
>  
> +struct fw_name_devm {
> +	unsigned long magic;
> +	char name[];
> +};
> +
>  #define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
>  
>  /* fw_lock could be moved to 'struct firmware_priv' but since it is just
> @@ -574,6 +579,56 @@ static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
>  	fw->priv = buf;
>  }
>  
> +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);

Alignment and stray newline.

> +
> +	if (!fwn)
> +		return -ENOMEM;
> +
> +	fwn->magic = (unsigned long)&fw_cache;
> +	strcpy(fwn->name, name);
> +	devres_add(dev, fwn);
> +
> +	return 0;
> +}
> +
>  static void _request_firmware_cleanup(const struct firmware **firmware_p)
>  {
>  	release_firmware(*firmware_p);
> @@ -690,6 +745,17 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>  
>  	fw_set_page_data(buf, fw_priv->fw);
>  
> +	/*
> +	 * add firmware name into devres list so that we can auto cache
> +	 * firmware for device.
> +	 *
> +	 * f_dev->parent may has been deleted already, but the problem
> +	 * should be fixed in devres.
> +	 *
> +	 */
> +	if (!retval && f_dev->parent)
> +		fw_add_devm_name(f_dev->parent, buf->fw_id);
> +
>  	fw_priv->buf = NULL;
>  	mutex_unlock(&fw_lock);
>  
> -- 
> 1.7.9.5
> 
> 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 09/13] driver core: firmware loader: store firmware name into devres list

[Ming Lei]

On Thu, Jul 26, 2012 at 12:15 AM, Borislav Petkov <bp@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:09AM +0800, Ming Lei wrote:
>> This patch will store firmware name into devres list of the device
>> which is requesting firmware loading, so that we can implement
>> auto cache firmware for devices in need.
>
> Stupid question: does this mean that once the firmware name is in the
> devres list, it is being cached automatically and device drivers which
> don't want that need to explicitly uncache it?

Both the auto cache and auto uncache actions are not triggered by device
driver, and will be triggered by some system state, for example, in 13/13,
you will find the cache is done before system suspend and the uncache is
done after system resume.

If device drivers want to cache its firmware explicitly, they have to uncache
it explicitly too, see cache_firmware/uncache_firmware in 7/13.

Thanks
--
Ming Lei

[RFC PATCH 10/13] driver core: devres: introduce devres_for_each_res

[Ming Lei]

This patch introduces one devres API of devres_for_each_res
so that the device's driver can iterate each resource it has
interest in.

The firmware loader will use the API to get each firmware name
from the device instance.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/devres.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    3 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 2360adb..8273ba5 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -144,6 +144,48 @@ EXPORT_SYMBOL_GPL(devres_alloc);
 #endif
 
 /**
+ * devres_for_each_res - Resource iterator
+ * @dev: Device to iterate resource from
+ * @release: Look for resources associated with this release function
+ * @match: Match function (optional)
+ * @match_data: Data for the match function
+ * @fn: function to be called for each matched resource.
+ *
+ * Call @fn for each devres of @dev which is associated with @release
+ * and for which @match returns 1.
+ *
+ * RETURNS:
+ * 	void
+ */
+void devres_for_each_res(struct device *dev, dr_release_t release,
+			dr_match_t match, void *match_data,
+			void (*fn)(struct device *, void *))
+{
+	struct devres_node *node;
+	struct devres_node *tmp;
+	unsigned long flags;
+
+	if (!fn)
+		return;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+	list_for_each_entry_safe_reverse(node, tmp,
+			&dev->devres_head, entry) {
+		struct devres *dr = container_of(node, struct devres, node);
+
+		if (node->release != release)
+			continue;
+		if (match && !match(dev, dr->data, match_data))
+			continue;
+		spin_unlock_irqrestore(&dev->devres_lock, flags);
+		fn(dev, dr->data);
+		spin_lock_irqsave(&dev->devres_lock, flags);
+	}
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+}
+EXPORT_SYMBOL_GPL(devres_for_each_res);
+
+/**
  * devres_free - Free device resource data
  * @res: Pointer to devres data to free
  *
diff --git a/include/linux/device.h b/include/linux/device.h
index 52a5f15..34dc1f8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -536,6 +536,9 @@ extern void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
 #else
 extern void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp);
 #endif
+extern void devres_for_each_res(struct device *dev, dr_release_t release,
+			dr_match_t match, void *match_data,
+			void (*fn)(struct device *, void *));
 extern void devres_free(void *res);
 extern void devres_add(struct device *dev, void *res);
 extern void *devres_find(struct device *dev, dr_release_t release,
-- 
1.7.9.5

Re: [RFC PATCH 10/13] driver core: devres: introduce devres_for_each_res

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:10AM +0800, Ming Lei wrote:
> This patch introduces one devres API of devres_for_each_res
> so that the device's driver can iterate each resource it has
> interest in.
> 
> The firmware loader will use the API to get each firmware name
> from the device instance.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/devres.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |    3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 2360adb..8273ba5 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -144,6 +144,48 @@ EXPORT_SYMBOL_GPL(devres_alloc);
>  #endif
>  
>  /**
> + * devres_for_each_res - Resource iterator
> + * @dev: Device to iterate resource from
> + * @release: Look for resources associated with this release function
> + * @match: Match function (optional)
> + * @match_data: Data for the match function
> + * @fn: function to be called for each matched resource.
> + *
> + * Call @fn for each devres of @dev which is associated with @release
> + * and for which @match returns 1.
> + *
> + * RETURNS:
> + * 	void
> + */
> +void devres_for_each_res(struct device *dev, dr_release_t release,
> +			dr_match_t match, void *match_data,
> +			void (*fn)(struct device *, void *))
> +{
> +	struct devres_node *node;
> +	struct devres_node *tmp;
> +	unsigned long flags;
> +
> +	if (!fn)
> +		return;
> +
> +	spin_lock_irqsave(&dev->devres_lock, flags);
> +	list_for_each_entry_safe_reverse(node, tmp,
> +			&dev->devres_head, entry) {

Why break this line?

	list_for_each_entry_safe_reverse(node, tmp, &dev->devres_head, entry) {

is perfectly fine.

> +		struct devres *dr = container_of(node, struct devres, node);
> +
> +		if (node->release != release)
> +			continue;
> +		if (match && !match(dev, dr->data, match_data))
> +			continue;
> +		spin_unlock_irqrestore(&dev->devres_lock, flags);
> +		fn(dev, dr->data);
> +		spin_lock_irqsave(&dev->devres_lock, flags);
> +	}
> +	spin_unlock_irqrestore(&dev->devres_lock, flags);

This looks strange. For the last node on the list, we're grabbing the
lock and releasing it right after.

Probably check whether node is the last element and only re-grab the
lock if it isn't?

And btw, don't we need to hold the ->devres_lock during the whole search
like callers of find_dr do, for example?

> +}
> +EXPORT_SYMBOL_GPL(devres_for_each_res);
> +
> +/**
>   * devres_free - Free device resource data
>   * @res: Pointer to devres data to free
>   *
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 52a5f15..34dc1f8 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -536,6 +536,9 @@ extern void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
>  #else
>  extern void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp);
>  #endif
> +extern void devres_for_each_res(struct device *dev, dr_release_t release,
> +			dr_match_t match, void *match_data,
> +			void (*fn)(struct device *, void *));

Args alignment.

>  extern void devres_free(void *res);
>  extern void devres_add(struct device *dev, void *res);
>  extern void *devres_find(struct device *dev, dr_release_t release,
> -- 
> 1.7.9.5
> 
> 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 10/13] driver core: devres: introduce devres_for_each_res

[Ming Lei]

On Thu, Jul 26, 2012 at 12:25 AM, Borislav Petkov <bp@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:10AM +0800, Ming Lei wrote:
>> This patch introduces one devres API of devres_for_each_res
>> so that the device's driver can iterate each resource it has
>> interest in.
>>
>> The firmware loader will use the API to get each firmware name
>> from the device instance.
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/base/devres.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/device.h |    3 +++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>> index 2360adb..8273ba5 100644
>> --- a/drivers/base/devres.c
>> +++ b/drivers/base/devres.c
>> @@ -144,6 +144,48 @@ EXPORT_SYMBOL_GPL(devres_alloc);
>>  #endif
>>
>>  /**
>> + * devres_for_each_res - Resource iterator
>> + * @dev: Device to iterate resource from
>> + * @release: Look for resources associated with this release function
>> + * @match: Match function (optional)
>> + * @match_data: Data for the match function
>> + * @fn: function to be called for each matched resource.
>> + *
>> + * Call @fn for each devres of @dev which is associated with @release
>> + * and for which @match returns 1.
>> + *
>> + * RETURNS:
>> + *   void
>> + */
>> +void devres_for_each_res(struct device *dev, dr_release_t release,
>> +                     dr_match_t match, void *match_data,
>> +                     void (*fn)(struct device *, void *))
>> +{
>> +     struct devres_node *node;
>> +     struct devres_node *tmp;
>> +     unsigned long flags;
>> +
>> +     if (!fn)
>> +             return;
>> +
>> +     spin_lock_irqsave(&dev->devres_lock, flags);
>> +     list_for_each_entry_safe_reverse(node, tmp,
>> +                     &dev->devres_head, entry) {
>
> Why break this line?
>
>         list_for_each_entry_safe_reverse(node, tmp, &dev->devres_head, entry) {
>
> is perfectly fine.
>
>> +             struct devres *dr = container_of(node, struct devres, node);
>> +
>> +             if (node->release != release)
>> +                     continue;
>> +             if (match && !match(dev, dr->data, match_data))
>> +                     continue;
>> +             spin_unlock_irqrestore(&dev->devres_lock, flags);
>> +             fn(dev, dr->data);
>> +             spin_lock_irqsave(&dev->devres_lock, flags);
>> +     }
>> +     spin_unlock_irqrestore(&dev->devres_lock, flags);
>
> This looks strange. For the last node on the list, we're grabbing the
> lock and releasing it right after.
>
> Probably check whether node is the last element and only re-grab the
> lock if it isn't?

It is not necessary since the lock isn't held in hot path.

>
> And btw, don't we need to hold the ->devres_lock during the whole search
> like callers of find_dr do, for example?

Because I don't want to put more constraint on the 'fn' about lock use, memory
allocation flag(gfp)..., from the view of API's user.

In fact, there is problem wrt. releasing lock since add_dr may add new node
during releasing lock.

So I plan to just hold the lock to cover calling 'fn' in -v1 instead
of using rcu list
helpers, which may introduce a lot change on devres.

Anyway the callers can copy the resources interested into a temporary list
in 'fn' and handle it later, which may simplify devres_for_each_res and other
devres helpers a lot.

Also I will introduce another parameter of 'void *data' to 'fn' in -v1.

Thanks,
--
Ming Lei

[RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares

[Ming Lei]

This patches introduces the three helpers below:

	void device_cache_firmwares(void)
	void device_uncache_firmwares(void)
	void device_uncache_firmwares_delay(unsigned long)

so we can call device_cache_firmwares() to cache firmwares for
all devices which need firmwares to work, and the device driver
can get the firmware easily from kernel memory when system isn't
readly for completing their requests of loading firmwares.

When system is ready for completing firmware loading, driver core
can call device_uncache_firmwares() or its delay version to free
the cached firmwares.

The above helpers should be used to cache device firmwares during
system suspend/resume cycle in the following patches.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |  236 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 230 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c181e6d..7a96e75 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -22,6 +22,10 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/list.h>
+#include <linux/async.h>
+#include <linux/pm.h>
+
+#include "base.h"
 
 MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
@@ -91,6 +95,17 @@ struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
 	struct list_head head;
+
+	/*
+	 * Name of firmware which has been cached successfully will be
+	 * added into the below list so that device uncache helper can
+	 * trace which firmware has been cached before.
+	 */
+	spinlock_t name_lock;
+	struct list_head name_head;
+	wait_queue_head_t      wait_queue;
+	int cnt;
+	struct delayed_work work;
 };
 
 struct firmware_buf {
@@ -107,6 +122,11 @@ struct firmware_buf {
 	char fw_id[];
 };
 
+struct fw_name_for_cache {
+	struct list_head list;
+	char name[];
+};
+
 struct firmware_priv {
 	struct timer_list timeout;
 	bool nowait;
@@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
 	kref_put(&buf->ref, __fw_free_buf);
 }
 
-static void fw_cache_init(void)
-{
-	spin_lock_init(&fw_cache.lock);
-	INIT_LIST_HEAD(&fw_cache.head);
-}
-
 static struct firmware_priv *to_firmware_priv(struct device *dev)
 {
 	return container_of(dev, struct firmware_priv, dev);
@@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
 	return -EINVAL;
 }
 
+static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
+{
+	struct fw_name_for_cache *nc;
+
+	nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
+	if (!nc)
+		goto exit;
+
+	strcpy(nc->name, name);
+exit:
+	return nc;
+}
+
+static void free_fw_name_cache(struct fw_name_for_cache *nc)
+{
+	kfree(nc);
+}
+
+static void __async_dev_cache_firmware(void *fw_name,
+		async_cookie_t cookie)
+{
+	struct fw_name_for_cache *nc;
+	struct firmware_cache *fwc = &fw_cache;
+	char *curr_name;
+	int ret;
+
+	/* 'fw_name' is stored in devres, and it may be released,
+	 * so allocate buffer to store the firmware name
+	 */
+	curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
+	if (!curr_name) {
+		ret = -ENOMEM;
+		goto drop_ref;
+	}
+
+	strcpy(curr_name, fw_name);
+
+	ret = cache_firmware(curr_name);
+
+	if (!ret) {
+		/*
+		 * allocate/all the instance of alloc_fw_name_cache
+		 * for uncaching later if cache_firmware returns
+		 * successfully
+		 */
+		nc = alloc_fw_name_cache(curr_name);
+
+		/*
+		 * have to uncache firmware in case of allocation
+		 * failure since we can't trace the firmware cache
+		 * any more without the firmware name.
+		 */
+		if (!nc) {
+			uncache_firmware(curr_name);
+		} else {
+			spin_lock(&fwc->name_lock);
+			list_add(&nc->list, &fwc->name_head);
+			spin_unlock(&fwc->name_lock);
+		}
+	}
+	kfree(curr_name);
+
+drop_ref:
+	spin_lock(&fwc->name_lock);
+	fwc->cnt--;
+	spin_unlock(&fwc->name_lock);
+	wake_up(&fwc->wait_queue);
+}
+
+static void __dev_cache_firmware(struct device *dev, void *res)
+{
+	struct fw_name_devm *fwn = res;
+	const char *fw_name = fwn->name;
+	struct firmware_cache *fwc = &fw_cache;
+
+	dev_dbg(dev, "fw-%s %d\n", fw_name, fwc->cnt);
+
+	spin_lock(&fwc->name_lock);
+	fwc->cnt++;
+	spin_unlock(&fwc->name_lock);
+
+	async_schedule(__async_dev_cache_firmware, (void *)fw_name);
+}
+
+static int devm_name_match(struct device *dev, void *res,
+		void *match_data)
+{
+	struct fw_name_devm *fwn = res;
+	return (fwn->magic == (unsigned long)match_data);
+}
+
+static void dev_cache_firmware(struct device *dev)
+{
+	devres_for_each_res(dev, fw_name_devm_release,
+			devm_name_match, &fw_cache,
+			__dev_cache_firmware);
+}
+
+static void __device_uncache_firmwares(void)
+{
+	struct firmware_cache *fwc = &fw_cache;
+	struct fw_name_for_cache *nc;
+
+	spin_lock(&fwc->name_lock);
+	while (!list_empty(&fwc->name_head)) {
+		nc = list_entry(fwc->name_head.next,
+				struct fw_name_for_cache, list);
+		list_del(&nc->list);
+		spin_unlock(&fwc->name_lock);
+
+		uncache_firmware(nc->name);
+		free_fw_name_cache(nc);
+
+		spin_lock(&fwc->name_lock);
+	}
+	spin_unlock(&fwc->name_lock);
+}
+
+extern struct list_head dpm_list;
+/**
+ * device_cache_firmwares - cache devices' firmwares
+ *
+ * For each devices, if they called request_firmware or
+ * request_firmware_nowait successfully before, their firmware
+ * name will be recored into these devices' devres link list, so
+ * device_cache_firmwares can call cache_firmware() to cache these
+ * firmwares for these devices, then these device drivers can load
+ * their firmwares easily at any time even when system is not ready
+ * to complete loading firmwares.
+ *
+ */
+static void device_cache_firmwares(void)
+{
+	struct firmware_cache *fwc = &fw_cache;
+	struct device *dev;
+	DEFINE_WAIT(wait);
+
+	pr_debug("%s\n", __func__);
+
+	device_pm_lock();
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		dev_cache_firmware(dev);
+	device_pm_unlock();
+
+	pr_debug("%s firmwares %d\n", __func__, fwc->cnt);
+
+	/* wait for completion of caching firmware for all devices */
+	spin_lock(&fwc->name_lock);
+	for (;;) {
+		prepare_to_wait(&fwc->wait_queue, &wait,
+				TASK_UNINTERRUPTIBLE);
+		if (!fwc->cnt)
+			break;
+
+		spin_unlock(&fwc->name_lock);
+
+		schedule();
+
+		spin_lock(&fwc->name_lock);
+	}
+	spin_unlock(&fwc->name_lock);
+	finish_wait(&fwc->wait_queue, &wait);
+}
+
+/**
+ * device_uncache_firmwares - uncache devices' firmwares
+ *
+ * uncache all firmwares which have been cached successfully
+ * by device_uncache_firmwares
+ *
+ */
+static void device_uncache_firmwares(void)
+{
+	pr_debug("%s\n", __func__);
+	__device_uncache_firmwares();
+}
+
+static void device_uncache_firmwares_work(struct work_struct *work)
+{
+	device_uncache_firmwares();
+}
+
+/**
+ * device_uncache_firmwares_delay - uncache devices firmwares
+ * @delay: number of milliseconds to delay uncache device firmwares
+ *
+ * uncache all devices's firmwares which has been cached successfully
+ * by device_cache_firmwares after @delay milliseconds.
+ *
+ */
+static void device_uncache_firmwares_delay(unsigned long delay)
+{
+	schedule_delayed_work(&fw_cache.work,
+			msecs_to_jiffies(delay));
+}
+
+static void __init fw_cache_init(void)
+{
+	spin_lock_init(&fw_cache.lock);
+	INIT_LIST_HEAD(&fw_cache.head);
+
+	spin_lock_init(&fw_cache.name_lock);
+	INIT_LIST_HEAD(&fw_cache.name_head);
+	fw_cache.cnt = 0;
+
+	init_waitqueue_head(&fw_cache.wait_queue);
+	INIT_DELAYED_WORK(&fw_cache.work,
+		device_uncache_firmwares_work);
+}
+
 static int __init firmware_class_init(void)
 {
 	fw_cache_init();
-- 
1.7.9.5

Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
> This patches introduces the three helpers below:
> 
> 	void device_cache_firmwares(void)
> 	void device_uncache_firmwares(void)
> 	void device_uncache_firmwares_delay(unsigned long)

I kinda don't like the plural of firmware: "firmwares". Can we call
those
	device_cache_fw_images
	device_uncache_fw_images

or
	device_cache_fw_blobs

or whatever?

> so we can call device_cache_firmwares() to cache firmwares for
> all devices which need firmwares to work, and the device driver
> can get the firmware easily from kernel memory when system isn't
> readly for completing their requests of loading firmwares.
> 
> When system is ready for completing firmware loading, driver core
> can call device_uncache_firmwares() or its delay version to free
> the cached firmwares.
> 
> The above helpers should be used to cache device firmwares during
> system suspend/resume cycle in the following patches.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |  236 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 230 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c181e6d..7a96e75 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -22,6 +22,10 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/list.h>
> +#include <linux/async.h>
> +#include <linux/pm.h>
> +
> +#include "base.h"
>  
>  MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -91,6 +95,17 @@ struct firmware_cache {
>  	/* firmware_buf instance will be added into the below list */
>  	spinlock_t lock;
>  	struct list_head head;
> +
> +	/*
> +	 * Name of firmware which has been cached successfully will be
> +	 * added into the below list so that device uncache helper can
> +	 * trace which firmware has been cached before.
> +	 */

Comment above the list_head and maybe call the list_head fw_names or so?

> +	spinlock_t name_lock;
> +	struct list_head name_head;
> +	wait_queue_head_t      wait_queue;

Stray \t

> +	int cnt;
> +	struct delayed_work work;
>  };
>  
>  struct firmware_buf {
> @@ -107,6 +122,11 @@ struct firmware_buf {
>  	char fw_id[];
>  };
>  
> +struct fw_name_for_cache {
> +	struct list_head list;
> +	char name[];
> +};

Maybe fw_cache_entry?

> +
>  struct firmware_priv {
>  	struct timer_list timeout;
>  	bool nowait;
> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
>  	kref_put(&buf->ref, __fw_free_buf);
>  }
>  
> -static void fw_cache_init(void)
> -{
> -	spin_lock_init(&fw_cache.lock);
> -	INIT_LIST_HEAD(&fw_cache.head);
> -}
> -
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
>  {
>  	return container_of(dev, struct firmware_priv, dev);
> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
>  	return -EINVAL;
>  }
>  
> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
> +{
> +	struct fw_name_for_cache *nc;
> +
> +	nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
> +	if (!nc)
> +		goto exit;
> +
> +	strcpy(nc->name, name);
> +exit:
> +	return nc;
> +}
> +
> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
> +{
> +	kfree(nc);
> +}
> +
> +static void __async_dev_cache_firmware(void *fw_name,
> +		async_cookie_t cookie)

Arg alignment.

> +{
> +	struct fw_name_for_cache *nc;
> +	struct firmware_cache *fwc = &fw_cache;
> +	char *curr_name;
> +	int ret;
> +
> +	/* 'fw_name' is stored in devres, and it may be released,
> +	 * so allocate buffer to store the firmware name
> +	 */
> +	curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
> +	if (!curr_name) {
> +		ret = -ENOMEM;
> +		goto drop_ref;
> +	}
> +
> +	strcpy(curr_name, fw_name);

AFAICT kstrdup already copies the existing string, why do you need to
strcpy it again?

> +
> +	ret = cache_firmware(curr_name);
> +
> +	if (!ret) {

Invert check logic to save an indentation level:

	if (ret)
		goto free;

	nc = alloc...

	...

 free:
 	kfree(curr_name);

> +		/*
> +		 * allocate/all the instance of alloc_fw_name_cache
> +		 * for uncaching later if cache_firmware returns
> +		 * successfully
> +		 */
> +		nc = alloc_fw_name_cache(curr_name);
> +
> +		/*
> +		 * have to uncache firmware in case of allocation
> +		 * failure since we can't trace the firmware cache
> +		 * any more without the firmware name.
> +		 */
> +		if (!nc) {
> +			uncache_firmware(curr_name);
> +		} else {
> +			spin_lock(&fwc->name_lock);
> +			list_add(&nc->list, &fwc->name_head);
> +			spin_unlock(&fwc->name_lock);
> +		}
> +	}
> +	kfree(curr_name);
> +
> +drop_ref:
> +	spin_lock(&fwc->name_lock);
> +	fwc->cnt--;
> +	spin_unlock(&fwc->name_lock);
> +	wake_up(&fwc->wait_queue);
> +}
> +
> +static void __dev_cache_firmware(struct device *dev, void *res)
> +{
> +	struct fw_name_devm *fwn = res;
> +	const char *fw_name = fwn->name;
> +	struct firmware_cache *fwc = &fw_cache;
> +
> +	dev_dbg(dev, "fw-%s %d\n", fw_name, fwc->cnt);
> +
> +	spin_lock(&fwc->name_lock);
> +	fwc->cnt++;
> +	spin_unlock(&fwc->name_lock);
> +
> +	async_schedule(__async_dev_cache_firmware, (void *)fw_name);
> +}
> +
> +static int devm_name_match(struct device *dev, void *res,
> +		void *match_data)

arg alignment

> +{
> +	struct fw_name_devm *fwn = res;
> +	return (fwn->magic == (unsigned long)match_data);
> +}
> +
> +static void dev_cache_firmware(struct device *dev)
> +{
> +	devres_for_each_res(dev, fw_name_devm_release,
> +			devm_name_match, &fw_cache,
> +			__dev_cache_firmware);

arg alignment

> +}
> +
> +static void __device_uncache_firmwares(void)
> +{
> +	struct firmware_cache *fwc = &fw_cache;
> +	struct fw_name_for_cache *nc;
> +
> +	spin_lock(&fwc->name_lock);
> +	while (!list_empty(&fwc->name_head)) {
> +		nc = list_entry(fwc->name_head.next,
> +				struct fw_name_for_cache, list);
> +		list_del(&nc->list);
> +		spin_unlock(&fwc->name_lock);
> +
> +		uncache_firmware(nc->name);
> +		free_fw_name_cache(nc);
> +
> +		spin_lock(&fwc->name_lock);
> +	}
> +	spin_unlock(&fwc->name_lock);

Same thing here: maybe check if on the last element on the list and
don't grab the lock then?

> +}
> +
> +extern struct list_head dpm_list;

checkpatch says here:

WARNING: externs should be avoided in .c files
#181: FILE: drivers/base/firmware_class.c:1118:
+extern struct list_head dpm_list;

and I think it is correct. AFAICT, it should be included from
<drivers/base/power/power.h>...

> +/**
> + * device_cache_firmwares - cache devices' firmwares
> + *
> + * For each devices, if they called request_firmware or
> + * request_firmware_nowait successfully before, their firmware
> + * name will be recored into these devices' devres link list, so
> + * device_cache_firmwares can call cache_firmware() to cache these
> + * firmwares for these devices, then these device drivers can load
> + * their firmwares easily at any time even when system is not ready
> + * to complete loading firmwares.

This is a one looong sentence. Please simplify.

> + *
> + */
> +static void device_cache_firmwares(void)
> +{
> +	struct firmware_cache *fwc = &fw_cache;
> +	struct device *dev;
> +	DEFINE_WAIT(wait);
> +
> +	pr_debug("%s\n", __func__);

No real need for that debug printk since you have one below.

> +
> +	device_pm_lock();
> +	list_for_each_entry(dev, &dpm_list, power.entry)
> +		dev_cache_firmware(dev);
> +	device_pm_unlock();
> +
> +	pr_debug("%s firmwares %d\n", __func__, fwc->cnt);
> +
> +	/* wait for completion of caching firmware for all devices */
> +	spin_lock(&fwc->name_lock);
> +	for (;;) {
> +		prepare_to_wait(&fwc->wait_queue, &wait,
> +				TASK_UNINTERRUPTIBLE);
> +		if (!fwc->cnt)
> +			break;
> +
> +		spin_unlock(&fwc->name_lock);
> +
> +		schedule();
> +
> +		spin_lock(&fwc->name_lock);
> +	}
> +	spin_unlock(&fwc->name_lock);
> +	finish_wait(&fwc->wait_queue, &wait);
> +}
> +
> +/**
> + * device_uncache_firmwares - uncache devices' firmwares
> + *
> + * uncache all firmwares which have been cached successfully
> + * by device_uncache_firmwares

    "by device_cache_firmwares earlier."

> + *
> + */
> +static void device_uncache_firmwares(void)
> +{
> +	pr_debug("%s\n", __func__);
> +	__device_uncache_firmwares();
> +}
> +

Ok, the rest of the patches tomorrow.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares

[Ming Lei]

On Thu, Jul 26, 2012 at 12:52 AM, Borislav Petkov <bp@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
>> This patches introduces the three helpers below:
>>
>>       void device_cache_firmwares(void)
>>       void device_uncache_firmwares(void)
>>       void device_uncache_firmwares_delay(unsigned long)
>
> I kinda don't like the plural of firmware: "firmwares". Can we call
> those
>         device_cache_fw_images
>         device_uncache_fw_images

Looks fine.

>
> or
>         device_cache_fw_blobs
>
> or whatever?
>
>> so we can call device_cache_firmwares() to cache firmwares for
>> all devices which need firmwares to work, and the device driver
>> can get the firmware easily from kernel memory when system isn't
>> readly for completing their requests of loading firmwares.
>>
>> When system is ready for completing firmware loading, driver core
>> can call device_uncache_firmwares() or its delay version to free
>> the cached firmwares.
>>
>> The above helpers should be used to cache device firmwares during
>> system suspend/resume cycle in the following patches.
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/base/firmware_class.c |  236 +++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 230 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index c181e6d..7a96e75 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/slab.h>
>>  #include <linux/sched.h>
>>  #include <linux/list.h>
>> +#include <linux/async.h>
>> +#include <linux/pm.h>
>> +
>> +#include "base.h"
>>
>>  MODULE_AUTHOR("Manuel Estrada Sainz");
>>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
>> @@ -91,6 +95,17 @@ struct firmware_cache {
>>       /* firmware_buf instance will be added into the below list */
>>       spinlock_t lock;
>>       struct list_head head;
>> +
>> +     /*
>> +      * Name of firmware which has been cached successfully will be
>> +      * added into the below list so that device uncache helper can
>> +      * trace which firmware has been cached before.
>> +      */
>
> Comment above the list_head and maybe call the list_head fw_names or so?
>
>> +     spinlock_t name_lock;
>> +     struct list_head name_head;
>> +     wait_queue_head_t      wait_queue;
>
> Stray \t
>
>> +     int cnt;
>> +     struct delayed_work work;
>>  };
>>
>>  struct firmware_buf {
>> @@ -107,6 +122,11 @@ struct firmware_buf {
>>       char fw_id[];
>>  };
>>
>> +struct fw_name_for_cache {
>> +     struct list_head list;
>> +     char name[];
>> +};
>
> Maybe fw_cache_entry?

Looks fine.

>
>> +
>>  struct firmware_priv {
>>       struct timer_list timeout;
>>       bool nowait;
>> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
>>       kref_put(&buf->ref, __fw_free_buf);
>>  }
>>
>> -static void fw_cache_init(void)
>> -{
>> -     spin_lock_init(&fw_cache.lock);
>> -     INIT_LIST_HEAD(&fw_cache.head);
>> -}
>> -
>>  static struct firmware_priv *to_firmware_priv(struct device *dev)
>>  {
>>       return container_of(dev, struct firmware_priv, dev);
>> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
>>       return -EINVAL;
>>  }
>>
>> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
>> +{
>> +     struct fw_name_for_cache *nc;
>> +
>> +     nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
>> +     if (!nc)
>> +             goto exit;
>> +
>> +     strcpy(nc->name, name);
>> +exit:
>> +     return nc;
>> +}
>> +
>> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
>> +{
>> +     kfree(nc);
>> +}
>> +
>> +static void __async_dev_cache_firmware(void *fw_name,
>> +             async_cookie_t cookie)
>
> Arg alignment.

I am wondering why checkpatch.pl doesn't add the check...

>
>> +{
>> +     struct fw_name_for_cache *nc;
>> +     struct firmware_cache *fwc = &fw_cache;
>> +     char *curr_name;
>> +     int ret;
>> +
>> +     /* 'fw_name' is stored in devres, and it may be released,
>> +      * so allocate buffer to store the firmware name
>> +      */
>> +     curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
>> +     if (!curr_name) {
>> +             ret = -ENOMEM;
>> +             goto drop_ref;
>> +     }
>> +
>> +     strcpy(curr_name, fw_name);
>
> AFAICT kstrdup already copies the existing string, why do you need to
> strcpy it again?

Good catch.

>
>> +
>> +     ret = cache_firmware(curr_name);
>> +
>> +     if (!ret) {
>
> Invert check logic to save an indentation level:
>
>         if (ret)
>                 goto free;
>
>         nc = alloc...
>
>         ...
>
>  free:
>         kfree(curr_name);
>
>> +             /*
>> +              * allocate/all the instance of alloc_fw_name_cache
>> +              * for uncaching later if cache_firmware returns
>> +              * successfully
>> +              */
>> +             nc = alloc_fw_name_cache(curr_name);
>> +
>> +             /*
>> +              * have to uncache firmware in case of allocation
>> +              * failure since we can't trace the firmware cache
>> +              * any more without the firmware name.
>> +              */
>> +             if (!nc) {
>> +                     uncache_firmware(curr_name);
>> +             } else {
>> +                     spin_lock(&fwc->name_lock);
>> +                     list_add(&nc->list, &fwc->name_head);
>> +                     spin_unlock(&fwc->name_lock);
>> +             }
>> +     }
>> +     kfree(curr_name);
>> +
>> +drop_ref:
>> +     spin_lock(&fwc->name_lock);
>> +     fwc->cnt--;
>> +     spin_unlock(&fwc->name_lock);
>> +     wake_up(&fwc->wait_queue);
>> +}
>> +
>> +static void __dev_cache_firmware(struct device *dev, void *res)
>> +{
>> +     struct fw_name_devm *fwn = res;
>> +     const char *fw_name = fwn->name;
>> +     struct firmware_cache *fwc = &fw_cache;
>> +
>> +     dev_dbg(dev, "fw-%s %d\n", fw_name, fwc->cnt);
>> +
>> +     spin_lock(&fwc->name_lock);
>> +     fwc->cnt++;
>> +     spin_unlock(&fwc->name_lock);
>> +
>> +     async_schedule(__async_dev_cache_firmware, (void *)fw_name);
>> +}
>> +
>> +static int devm_name_match(struct device *dev, void *res,
>> +             void *match_data)
>
> arg alignment
>
>> +{
>> +     struct fw_name_devm *fwn = res;
>> +     return (fwn->magic == (unsigned long)match_data);
>> +}
>> +
>> +static void dev_cache_firmware(struct device *dev)
>> +{
>> +     devres_for_each_res(dev, fw_name_devm_release,
>> +                     devm_name_match, &fw_cache,
>> +                     __dev_cache_firmware);
>
> arg alignment
>
>> +}
>> +
>> +static void __device_uncache_firmwares(void)
>> +{
>> +     struct firmware_cache *fwc = &fw_cache;
>> +     struct fw_name_for_cache *nc;
>> +
>> +     spin_lock(&fwc->name_lock);
>> +     while (!list_empty(&fwc->name_head)) {
>> +             nc = list_entry(fwc->name_head.next,
>> +                             struct fw_name_for_cache, list);
>> +             list_del(&nc->list);
>> +             spin_unlock(&fwc->name_lock);
>> +
>> +             uncache_firmware(nc->name);
>> +             free_fw_name_cache(nc);
>> +
>> +             spin_lock(&fwc->name_lock);
>> +     }
>> +     spin_unlock(&fwc->name_lock);
>
> Same thing here: maybe check if on the last element on the list and
> don't grab the lock then?
>
>> +}
>> +
>> +extern struct list_head dpm_list;
>
> checkpatch says here:
>
> WARNING: externs should be avoided in .c files
> #181: FILE: drivers/base/firmware_class.c:1118:
> +extern struct list_head dpm_list;
>
> and I think it is correct. AFAICT, it should be included from
> <drivers/base/power/power.h>...

Good catch.

>
>> +/**
>> + * device_cache_firmwares - cache devices' firmwares
>> + *
>> + * For each devices, if they called request_firmware or
>> + * request_firmware_nowait successfully before, their firmware
>> + * name will be recored into these devices' devres link list, so
>> + * device_cache_firmwares can call cache_firmware() to cache these
>> + * firmwares for these devices, then these device drivers can load
>> + * their firmwares easily at any time even when system is not ready
>> + * to complete loading firmwares.
>
> This is a one looong sentence. Please simplify.
>
>> + *
>> + */
>> +static void device_cache_firmwares(void)
>> +{
>> +     struct firmware_cache *fwc = &fw_cache;
>> +     struct device *dev;
>> +     DEFINE_WAIT(wait);
>> +
>> +     pr_debug("%s\n", __func__);
>
> No real need for that debug printk since you have one below.

Suppose dev_cache_firmware hangs, you will see nothing without
the above line.

>
>> +
>> +     device_pm_lock();
>> +     list_for_each_entry(dev, &dpm_list, power.entry)
>> +             dev_cache_firmware(dev);
>> +     device_pm_unlock();
>> +
>> +     pr_debug("%s firmwares %d\n", __func__, fwc->cnt);
>> +
>> +     /* wait for completion of caching firmware for all devices */
>> +     spin_lock(&fwc->name_lock);
>> +     for (;;) {
>> +             prepare_to_wait(&fwc->wait_queue, &wait,
>> +                             TASK_UNINTERRUPTIBLE);
>> +             if (!fwc->cnt)
>> +                     break;
>> +
>> +             spin_unlock(&fwc->name_lock);
>> +
>> +             schedule();
>> +
>> +             spin_lock(&fwc->name_lock);
>> +     }
>> +     spin_unlock(&fwc->name_lock);
>> +     finish_wait(&fwc->wait_queue, &wait);
>> +}
>> +
>> +/**
>> + * device_uncache_firmwares - uncache devices' firmwares
>> + *
>> + * uncache all firmwares which have been cached successfully
>> + * by device_uncache_firmwares
>
>     "by device_cache_firmwares earlier."
>
>> + *
>> + */
>> +static void device_uncache_firmwares(void)
>> +{
>> +     pr_debug("%s\n", __func__);
>> +     __device_uncache_firmwares();
>> +}
>> +
>
> Ok, the rest of the patches tomorrow.

Thank you for review.

Thanks,
--
Ming Lei

[RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Ming Lei]

Because device_cache_firmwares only cache the firmware which has been
loaded sucessfully at leat once, using a small loading timeout should
be OK.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7a96e75..0918b26 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1130,10 +1130,19 @@ static void device_cache_firmwares(void)
 {
 	struct firmware_cache *fwc = &fw_cache;
 	struct device *dev;
+	int old_timeout;
 	DEFINE_WAIT(wait);
 
 	pr_debug("%s\n", __func__);
 
+	/*
+	 * use small loading timeout for caching devces firmwares
+	 * because all these firmwares have been loaded successfully
+	 * at lease once
+	 */
+	old_timeout = loading_timeout;
+	loading_timeout = 10;
+
 	device_pm_lock();
 	list_for_each_entry(dev, &dpm_list, power.entry)
 		dev_cache_firmware(dev);
@@ -1157,6 +1166,8 @@ static void device_cache_firmwares(void)
 	}
 	spin_unlock(&fwc->name_lock);
 	finish_wait(&fwc->wait_queue, &wait);
+
+	loading_timeout = old_timeout;
 }
 
 /**
-- 
1.7.9.5

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote:
> Because device_cache_firmwares only cache the firmware which has been
> loaded sucessfully at leat once, using a small loading timeout should

			least

> be OK.

Your commit message doesn't explain why exactly we decrease the timeout:
you should probably say that this patch overrides the default 60s
timeout because we're in pre-suspend/-hibernate mode where we have
userspace and are able to load the firmware quickly.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Ming Lei]

On Thu, Jul 26, 2012 at 8:36 PM, Borislav Petkov <bp@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote:
>> Because device_cache_firmwares only cache the firmware which has been
>> loaded sucessfully at leat once, using a small loading timeout should
>
>                         least
>
>> be OK.
>
> Your commit message doesn't explain why exactly we decrease the timeout:

I have explained it. Because the firmware has been loaded successfully at least
once, so it is very probably to not timeout.

> you should probably say that this patch overrides the default 60s
> timeout because we're in pre-suspend/-hibernate mode where we have
> userspace and are able to load the firmware quickly.

No, it is not what I was saying.

Thanks,
--
Ming Lei

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Borislav Petkov]

On Thu, Jul 26, 2012 at 11:48:17PM +0800, Ming Lei wrote:
> On Thu, Jul 26, 2012 at 8:36 PM, Borislav Petkov <bp@amd64.org> wrote:
> > On Wed, Jul 25, 2012 at 01:00:12AM +0800, Ming Lei wrote:
> >> Because device_cache_firmwares only cache the firmware which has been
> >> loaded sucessfully at leat once, using a small loading timeout should
> >
> >                         least
> >
> >> be OK.
> >
> > Your commit message doesn't explain why exactly we decrease the timeout:
> 
> I have explained it. Because the firmware has been loaded successfully at least
> once, so it is very probably to not timeout.
> 
> > you should probably say that this patch overrides the default 60s
> > timeout because we're in pre-suspend/-hibernate mode where we have
> > userspace and are able to load the firmware quickly.
> 
> No, it is not what I was saying.

Ok, maybe I'm not understanding this then. So explain to me this: why
do you need that timeout value of 10, how did we decide it to be 10
(and not 20 or 30 or whatever)? Generally, why do we need to reprogram
the timer to a smaller timeout instead of simply doing the completion
without a timeout?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Ming Lei]

On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov <bp@amd64.org> wrote:

>> No, it is not what I was saying.

I just mean the point is not mentioned in my commit log, but I admit it should
be a appropriate cause.

>
> Ok, maybe I'm not understanding this then. So explain to me this: why
> do you need that timeout value of 10, how did we decide it to be 10

If one firmware image was loaded successfully before, the probability of
loading it successfully at this time should be much higher than the 1st time
because something crazy(for example, the firmware is deleted) happens
with low probability.

Choosing 10 secs is just a estimation for loading time because the maximum
size of firmware in current distributions is about 2M bytes, since we know
it has been loaded successfully before.

> (and not 20 or 30 or whatever)? Generally, why do we need to reprogram
> the timer to a smaller timeout instead of simply doing the completion
> without a timeout?

No, it should be crazy without a timeout, and it can be triggered in init call
easily.


Thanks,
--
Ming Lei

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Borislav Petkov]

On Fri, Jul 27, 2012 at 09:54:25AM +0800, Ming Lei wrote:
> On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov <bp@amd64.org> wrote:
> 
> >> No, it is not what I was saying.
> 
> I just mean the point is not mentioned in my commit log, but I admit it should
> be a appropriate cause.
> 
> >
> > Ok, maybe I'm not understanding this then. So explain to me this: why
> > do you need that timeout value of 10, how did we decide it to be 10
> 
> If one firmware image was loaded successfully before, the probability of
> loading it successfully at this time should be much higher than the 1st time
> because something crazy(for example, the firmware is deleted) happens
> with low probability.

Believe it or not, I'm addressing exactly the possibility of the
firmware disappearing from under us in the AMD microcode driver
currently :) (and some other annoyances, of course).

> Choosing 10 secs is just a estimation for loading time because the maximum
> size of firmware in current distributions is about 2M bytes, since we know
> it has been loaded successfully before.

This is exactly the comment we want over the code to explain to others
why we're choosing 10 secs. Simply add that sentence above the 10s
assignment and we're perfect! :-)

> > (and not 20 or 30 or whatever)? Generally, why do we need to reprogram
> > the timer to a smaller timeout instead of simply doing the completion
> > without a timeout?
> 
> No, it should be crazy without a timeout, and it can be triggered in init call
> easily.

Ok.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 12/13] driver core: firmware loader: use small timeout for cache device firmware

[Ming Lei]

On Fri, Jul 27, 2012 at 6:35 PM, Borislav Petkov <bp@amd64.org> wrote:
> On Fri, Jul 27, 2012 at 09:54:25AM +0800, Ming Lei wrote:
>> On Fri, Jul 27, 2012 at 1:54 AM, Borislav Petkov <bp@amd64.org> wrote:
>>
>> >> No, it is not what I was saying.
>>
>> I just mean the point is not mentioned in my commit log, but I admit it should
>> be a appropriate cause.
>>
>> >
>> > Ok, maybe I'm not understanding this then. So explain to me this: why
>> > do you need that timeout value of 10, how did we decide it to be 10
>>
>> If one firmware image was loaded successfully before, the probability of
>> loading it successfully at this time should be much higher than the 1st time
>> because something crazy(for example, the firmware is deleted) happens
>> with low probability.
>
> Believe it or not, I'm addressing exactly the possibility of the
> firmware disappearing from under us in the AMD microcode driver
> currently :) (and some other annoyances, of course).

Of course, it is possible since user can delete it anytime, but with very
low probability.

>
>> Choosing 10 secs is just a estimation for loading time because the maximum
>> size of firmware in current distributions is about 2M bytes, since we know
>> it has been loaded successfully before.
>
> This is exactly the comment we want over the code to explain to others
> why we're choosing 10 secs. Simply add that sentence above the 10s
> assignment and we're perfect! :-)

OK, will add the comments in -v1.

Thanks,
--
Ming Lei

[RFC PATCH 13/13] driver core: firmware loader: cache devices firmware during suspend/resume cycle

[Ming Lei]

This patch implements caching devices' firmware automatically
during system syspend/resume cycle, so any device drivers can
call request_firmware or request_firmware_nowait inside resume
path to get the cached firmware if they have loaded firmwares
successfully at least once before entering suspend.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 0918b26..59384e0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -24,6 +24,7 @@
 #include <linux/list.h>
 #include <linux/async.h>
 #include <linux/pm.h>
+#include <linux/suspend.h>
 
 #include "base.h"
 
@@ -106,6 +107,8 @@ struct firmware_cache {
 	wait_queue_head_t      wait_queue;
 	int cnt;
 	struct delayed_work work;
+
+	struct notifier_block   pm_notify;
 };
 
 struct firmware_buf {
@@ -1202,6 +1205,31 @@ static void device_uncache_firmwares_delay(unsigned long delay)
 			msecs_to_jiffies(delay));
 }
 
+#ifdef CONFIG_PM
+static int fw_pm_notify(struct notifier_block *notify_block,
+					unsigned long mode, void *unused)
+{
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+		device_cache_firmwares();
+		break;
+
+	case PM_POST_SUSPEND:
+	case PM_POST_HIBERNATION:
+	case PM_POST_RESTORE:
+		device_uncache_firmwares_delay(10 * MSEC_PER_SEC);
+		break;
+	}
+
+	return 0;
+}
+#else
+static int fw_pm_notify(struct notifier_block *notify_block,
+					unsigned long mode, void *unused)
+{}
+#endif
+
 static void __init fw_cache_init(void)
 {
 	spin_lock_init(&fw_cache.lock);
@@ -1214,6 +1242,9 @@ static void __init fw_cache_init(void)
 	init_waitqueue_head(&fw_cache.wait_queue);
 	INIT_DELAYED_WORK(&fw_cache.work,
 		device_uncache_firmwares_work);
+	fw_cache.pm_notify.notifier_call = fw_pm_notify;
+
+	register_pm_notifier(&fw_cache.pm_notify);
 }
 
 static int __init firmware_class_init(void)
@@ -1224,6 +1255,7 @@ static int __init firmware_class_init(void)
 
 static void __exit firmware_class_exit(void)
 {
+	unregister_pm_notifier(&fw_cache.pm_notify);
 	class_unregister(&firmware_class);
 }
 
-- 
1.7.9.5

Re: [RFC PATCH 13/13] driver core: firmware loader: cache devices firmware during suspend/resume cycle

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:13AM +0800, Ming Lei wrote:
> This patch implements caching devices' firmware automatically
> during system syspend/resume cycle, so any device drivers can
> call request_firmware or request_firmware_nowait inside resume
> path to get the cached firmware if they have loaded firmwares
> successfully at least once before entering suspend.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/firmware_class.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 0918b26..59384e0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -24,6 +24,7 @@
>  #include <linux/list.h>
>  #include <linux/async.h>
>  #include <linux/pm.h>
> +#include <linux/suspend.h>
>  
>  #include "base.h"
>  
> @@ -106,6 +107,8 @@ struct firmware_cache {
>  	wait_queue_head_t      wait_queue;
>  	int cnt;
>  	struct delayed_work work;
> +

stray newline

> +	struct notifier_block   pm_notify;

variable names should be either all aligned or none at all but not
something in between.

>  };
>  
>  struct firmware_buf {
> @@ -1202,6 +1205,31 @@ static void device_uncache_firmwares_delay(unsigned long delay)
>  			msecs_to_jiffies(delay));
>  }
>  
> +#ifdef CONFIG_PM
> +static int fw_pm_notify(struct notifier_block *notify_block,
> +					unsigned long mode, void *unused)
> +{
> +	switch (mode) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +		device_cache_firmwares();
> +		break;
> +
> +	case PM_POST_SUSPEND:
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_RESTORE:
> +		device_uncache_firmwares_delay(10 * MSEC_PER_SEC);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int fw_pm_notify(struct notifier_block *notify_block,
> +					unsigned long mode, void *unused)
> +{}

static inline int fw_pm...

> +#endif
> +
>  static void __init fw_cache_init(void)
>  {
>  	spin_lock_init(&fw_cache.lock);
> @@ -1214,6 +1242,9 @@ static void __init fw_cache_init(void)
>  	init_waitqueue_head(&fw_cache.wait_queue);
>  	INIT_DELAYED_WORK(&fw_cache.work,
>  		device_uncache_firmwares_work);
> +	fw_cache.pm_notify.notifier_call = fw_pm_notify;
> +
> +	register_pm_notifier(&fw_cache.pm_notify);
>  }
>  
>  static int __init firmware_class_init(void)
> @@ -1224,6 +1255,7 @@ static int __init firmware_class_init(void)
>  
>  static void __exit firmware_class_exit(void)
>  {
> +	unregister_pm_notifier(&fw_cache.pm_notify);
>  	class_unregister(&firmware_class);
>  }

Ok, that should be it.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [RFC PATCH 13/13] driver core: firmware loader: cache devices firmware during suspend/resume cycle

[Ming Lei]

On Thu, Jul 26, 2012 at 8:43 PM, Borislav Petkov <bp@amd64.org> wrote:
>> +#else
>> +static int fw_pm_notify(struct notifier_block *notify_block,
>> +                                     unsigned long mode, void *unused)
>> +{}
>
> static inline int fw_pm...

Will add inline in -v1.


Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

On Wed, Jul 25, 2012 at 1:00 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Hi,
>
> In [1][2], the problem below has been discussed for some time:
>
>         device's firmware may be lost during suspend/resume
>         cycle because device is unplugged and plugged again
>         or device experiences system power loss in the period.
>         but during resume path, system is still not ready(process
>         frozen, rootfs not usable, ...) to complete loading firmware
>         from user space for the device
>
> The conclusion is that caching firmware during suspend/resume cycle
> is capable of solving the problem.
>
> This patchset implements cache/uncache firmware mechanism,
> and apply the mechnism to cache device's firmware in kernel memory
> space automatically during suspend/resume cyclye, so device can
> load its firmware easily during resume path. When resume is completed
> and system is ready, the cached firmwares will be removed from
> kernel memory later.

$git diff --stat
 drivers/base/devres.c         |   42 +++
 drivers/base/firmware_class.c |  749 +++++++++++++++++++++++++++++++++++------
 include/linux/device.h        |    3 +
 include/linux/firmware.h      |   15 +
 4 files changed, 705 insertions(+), 104 deletions(-)


Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Linus Torvalds]

On Tue, Jul 24, 2012 at 10:00 AM, Ming Lei <ming.lei@canonical.com> wrote:
>
> This patchset implements cache/uncache firmware mechanism,

Nothing in this patchset made me go "Eww", and several of them looked
like good cleanups, but maybe somebody else hates this approach.

              Linus

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

Cc Matthew

On Wed, Jul 25, 2012 at 1:16 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> Nothing in this patchset made me go "Eww", and several of them looked
> like good cleanups, but maybe somebody else hates this approach.

I remembered that Matthew objected caching firmware because he thought
it can't solve the isight camera problem after warm reset. But now,
this specific
problem can be solved easily by letting its driver defer probe.


Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Linus Torvalds]

On Tue, Jul 24, 2012 at 10:47 AM, Ming Lei <ming.lei@canonical.com> wrote:
>
> I remembered that Matthew objected caching firmware because he thought
> it can't solve the isight camera problem after warm reset. But now,
> this specific
> problem can be solved easily by letting its driver defer probe.

I really think the isight thing is a totally different thing entirely.

And quite frankly, that's just a BUG in the USB implementation. If the
USB ID changes, it shouldn't be considered a "resume" thing at all,
but a probe thing, and that should not be done in early resume - it
should be done *after* the resume is done.

The fact that USB confuses resuming existing devices and probing new
ones is just an implementation issue, and is a bug.

                       Linus

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Linus Torvalds]

On Tue, Jul 24, 2012 at 10:53 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The fact that USB confuses resuming existing devices and probing new
> ones is just an implementation issue, and is a bug.

Btw, I understand *why* it confuses the two. That doesn't make is less
of a bug, though.

                Linus

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

CC usb guys and list

On Wed, Jul 25, 2012 at 1:53 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I really think the isight thing is a totally different thing entirely.
>
> And quite frankly, that's just a BUG in the USB implementation. If the
> USB ID changes, it shouldn't be considered a "resume" thing at all,
> but a probe thing, and that should not be done in early resume - it
> should be done *after* the resume is done.

IMO, usbcore may have found the ID changes during resume(reset_resume),
and make the device disconnect. The disconnect event will be handled
in hubd kthread, which is woken up before usermodehelper_enable()(see
thaw_processes), so request_firmware will return failure during probe()
inside hubd kthread.

The cache firmware patch set may not help the situation, because the
original isight usb device for downloading firmware has been disconnected
before system suspend, so firmware loader can't cache the firmware for
the device.

The below patch should fix the problem above.

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 19db29f..eb8355f 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -185,16 +185,18 @@ void thaw_processes(void)

 	printk("Restarting tasks ... ");

-	thaw_workqueues();
-
 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
-		__thaw_task(p);
+		if (!(p->flags & (PF_KTHREAD | PF_WQ_WORKER)))
+			__thaw_task(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);

 	usermodehelper_enable();

+	/* let kthread see usermodehelper enabled flag */
+	thaw_kernel_threads();
+
 	schedule();
 	printk("done.\n");
 }


Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Oliver Neukum]

On Wednesday 25 July 2012 20:35:28 Ming Lei wrote:
> CC usb guys and list
> 
> On Wed, Jul 25, 2012 at 1:53 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > I really think the isight thing is a totally different thing entirely.
> >
> > And quite frankly, that's just a BUG in the USB implementation. If the
> > USB ID changes, it shouldn't be considered a "resume" thing at all,
> > but a probe thing, and that should not be done in early resume - it
> > should be done *after* the resume is done.
> 
> IMO, usbcore may have found the ID changes during resume(reset_resume),
> and make the device disconnect. The disconnect event will be handled
> in hubd kthread, which is woken up before usermodehelper_enable()(see
> thaw_processes), so request_firmware will return failure during probe()
> inside hubd kthread.
> 
> The cache firmware patch set may not help the situation, because the
> original isight usb device for downloading firmware has been disconnected
> before system suspend, so firmware loader can't cache the firmware for
> the device.
> 
> The below patch should fix the problem above.

This is likely unwise. You'd better introduce a special flag for kernel threads
that should be thawed only after user space will have been thawed.

	Regards
		Oliver

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

On Wed, Jul 25, 2012 at 8:43 PM, Oliver Neukum <oneukum@suse.de> wrote:
> This is likely unwise. You'd better introduce a special flag for kernel threads
> that should be thawed only after user space will have been thawed.

IMO, it is not necessary to introduce one extra flag for the purpose since

     - usermodehelper flag should be set/get as enabled after user space
       tasks have been waken up

     - no harm to thaw all user space tasks before thawing all kernel threads
     (there isn't any dependency about the thawing order)

Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

On Wed, Jul 25, 2012 at 8:50 PM, Ming Lei <ming.lei@canonical.com> wrote:
>      - no harm to thaw all user space tasks before thawing all kernel threads
>      (there isn't any dependency about the thawing order)

Sorry, I mean there isn't any constraint about the order, but the 'dependency'
may be just what the patch is introducing.


Thanks,
--
Ming Lei

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Linus Torvalds]

On Wed, Jul 25, 2012 at 5:35 AM, Ming Lei <ming.lei@canonical.com> wrote:
>
> The below patch should fix the problem above.

Actually, I think we could make this even simpler.

There's nothing wrong with saying "user mode is enabled" *just* before
we unthaw things, if we also simply guarantee that there is no
sleeping lock or similar that we might get caught on (causing a
deadlock or other untimely waking) before we've actually thawed
everything.

So *if* the only problem wrt the USB hub code comes from this area,
then I think the solution might be as simple as just moving the
"usermodehelper_enable()" up a few lines, with a comment. Something
like the *untested* and whitespace-damaged thing below..

Rafael? Who has one of those isight things and has seen the warning to test?

               Linus

---
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 19db29f67558..5bf50e488196 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -181,6 +181,12 @@ void thaw_processes(void)
        pm_freezing = false;
        pm_nosig_freezing = false;

+       /*
+        * User mode helper are available again (or will be,
+        * modulo scheduling)
+        */
+       usermodehelper_enable();
+
        oom_killer_enable();

        printk("Restarting tasks ... ");
@@ -193,8 +199,6 @@ void thaw_processes(void)
        } while_each_thread(g, p);
        read_unlock(&tasklist_lock);

-       usermodehelper_enable();
-
        schedule();
        printk("done.\n");
 }

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Rafael J. Wysocki]

On Wednesday, July 25, 2012, Linus Torvalds wrote:
> On Wed, Jul 25, 2012 at 5:35 AM, Ming Lei <ming.lei@canonical.com> wrote:
> >
> > The below patch should fix the problem above.
> 
> Actually, I think we could make this even simpler.
> 
> There's nothing wrong with saying "user mode is enabled" *just* before
> we unthaw things, if we also simply guarantee that there is no
> sleeping lock or similar that we might get caught on (causing a
> deadlock or other untimely waking) before we've actually thawed
> everything.
> 
> So *if* the only problem wrt the USB hub code comes from this area,
> then I think the solution might be as simple as just moving the
> "usermodehelper_enable()" up a few lines, with a comment. Something
> like the *untested* and whitespace-damaged thing below..
> 
> Rafael?

Yes, we can do this I think.

> Who has one of those isight things and has seen the warning to test?

That I can't answer, sorry.

Rafael


> ---
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 19db29f67558..5bf50e488196 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -181,6 +181,12 @@ void thaw_processes(void)
>         pm_freezing = false;
>         pm_nosig_freezing = false;
> 
> +       /*
> +        * User mode helper are available again (or will be,
> +        * modulo scheduling)
> +        */
> +       usermodehelper_enable();
> +
>         oom_killer_enable();
> 
>         printk("Restarting tasks ... ");
> @@ -193,8 +199,6 @@ void thaw_processes(void)
>         } while_each_thread(g, p);
>         read_unlock(&tasklist_lock);
> 
> -       usermodehelper_enable();
> -
>         schedule();
>         printk("done.\n");
>  }
> 
> 

Re: [RFC PATCH 00/13] firmware loader: introduce cache/uncache firmware

[Ming Lei]

On Thu, Jul 26, 2012 at 1:23 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So *if* the only problem wrt the USB hub code comes from this area,

IMO,  USB hub code may not be the only one because both device_add and
device_del can just be run in process context, so any hotplug bus
may have this kind of problem since the devices in the bus may be
unplugged and plugged again during suspend or experience power loss
, then the bus driver may take similar handling policy as USB.

> then I think the solution might be as simple as just moving the
> "usermodehelper_enable()" up a few lines, with a comment. Something
> like the *untested* and whitespace-damaged thing below..
>
> Rafael? Who has one of those isight things and has seen the warning to test?

I fake one USB device disconnect in reset_resume path and looks the patch
is good: the device driver can request firmware successfully in its .probe().

>
>                Linus
>
> ---
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 19db29f67558..5bf50e488196 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -181,6 +181,12 @@ void thaw_processes(void)
>         pm_freezing = false;
>         pm_nosig_freezing = false;
>
> +       /*
> +        * User mode helper are available again (or will be,
> +        * modulo scheduling)
> +        */
> +       usermodehelper_enable();

This may wake up tasks earlier than before, not sure if it might have
side effects.


Thanks,
--
Ming Lei