[PATCH 0/4] firmware loader: misc changes

[PATCH 0/4] firmware loader: misc changes

[Ming Lei]

Hi,

The first two are fixes' patch, the 3rd one is to enable
caching firmware for direct loading, and the last one is
a cleanup patch.

 drivers/base/firmware_class.c |  266 +++++++++++++++++++++++------------------
 1 file changed, 151 insertions(+), 115 deletions(-)


Thanks,
--
Ming Lei

[PATCH 1/4] firmware loader: cancel uncache work before caching firmware

[Ming Lei]

Under 'Opportunistic sleep' situation, system sleep might be
triggered very frequently, so the uncahce work may not be completed
before caching firmware during next suspend.

This patch cancels the uncache work before caching firmware to
fix the problem above.

Also this patch optimizes the cacheing firmware mechanism a bit by
only storing one firmware cache entry for one firmware image.

So if the firmware is still cached during suspend, it doesn't need
to be loaded from user space any more.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8154145..d06a8d0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1142,17 +1142,27 @@ exit:
 	return fce;
 }
 
-static int fw_cache_piggyback_on_request(const char *name)
+static int __fw_entry_found(const char *name)
 {
 	struct firmware_cache *fwc = &fw_cache;
 	struct fw_cache_entry *fce;
-	int ret = 0;
 
-	spin_lock(&fwc->name_lock);
 	list_for_each_entry(fce, &fwc->fw_names, list) {
 		if (!strcmp(fce->name, name))
-			goto found;
+			return 1;
 	}
+	return 0;
+}
+
+static int fw_cache_piggyback_on_request(const char *name)
+{
+	struct firmware_cache *fwc = &fw_cache;
+	struct fw_cache_entry *fce;
+	int ret = 0;
+
+	spin_lock(&fwc->name_lock);
+	if (__fw_entry_found(name))
+		goto found;
 
 	fce = alloc_fw_cache_entry(name);
 	if (fce) {
@@ -1229,11 +1239,19 @@ static void dev_cache_fw_image(struct device *dev, void *data)
 		list_del(&fce->list);
 
 		spin_lock(&fwc->name_lock);
-		fwc->cnt++;
-		list_add(&fce->list, &fwc->fw_names);
+		/* only one cache entry for one firmware */
+		if (!__fw_entry_found(fce->name)) {
+			fwc->cnt++;
+			list_add(&fce->list, &fwc->fw_names);
+		} else {
+			free_fw_cache_entry(fce);
+			fce = NULL;
+		}
 		spin_unlock(&fwc->name_lock);
 
-		async_schedule(__async_dev_cache_fw_image, (void *)fce);
+		if (fce)
+			async_schedule(__async_dev_cache_fw_image,
+				       (void *)fce);
 	}
 }
 
@@ -1275,6 +1293,9 @@ static void device_cache_fw_images(void)
 
 	pr_debug("%s\n", __func__);
 
+	/* cancel uncache work */
+	cancel_delayed_work_sync(&fwc->work);
+
 	/*
 	 * use small loading timeout for caching devices' firmware
 	 * because all these firmware images have been loaded
-- 
1.7.9.5

[PATCH 2/4] firmware loader: fix one reqeust_firmware race

[Ming Lei]

Several loading requests may be pending on one same
firmware buf, and this patch moves fw_map_pages_buf()
before complete_all(&fw_buf->completion) and let all
requests see the mapped 'buf->data' once the loading
is completed.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d06a8d0..f288251 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -423,6 +423,18 @@ static void firmware_free_data(const struct firmware *fw)
 #ifndef PAGE_KERNEL_RO
 #define PAGE_KERNEL_RO PAGE_KERNEL
 #endif
+
+/* one pages buffer should be mapped/unmapped only once */
+static int fw_map_pages_buf(struct firmware_buf *buf)
+{
+	if (buf->data)
+		vunmap(buf->data);
+	buf->data = vmap(buf->pages, buf->nr_pages, 0, PAGE_KERNEL_RO);
+	if (!buf->data)
+		return -ENOMEM;
+	return 0;
+}
+
 /**
  * firmware_loading_store - set value in the 'loading' control file
  * @dev: device pointer
@@ -467,6 +479,14 @@ 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);
+
+			/*
+			 * Several loading requests may be pending on
+			 * one same firmware buf, so let all requests
+			 * see the mapped 'buf->data' once the loading
+			 * is completed.
+			 * */
+			fw_map_pages_buf(fw_buf);
 			complete_all(&fw_buf->completion);
 			break;
 		}
@@ -670,15 +690,6 @@ exit:
 	return fw_priv;
 }
 
-/* one pages buffer is mapped/unmapped only once */
-static int fw_map_pages_buf(struct firmware_buf *buf)
-{
-	buf->data = vmap(buf->pages, buf->nr_pages, 0, PAGE_KERNEL_RO);
-	if (!buf->data)
-		return -ENOMEM;
-	return 0;
-}
-
 /* store the pages buffer info firmware from buf */
 static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
 {
@@ -884,9 +895,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	if (!retval && f_dev->parent)
 		fw_add_devm_name(f_dev->parent, buf->fw_id);
 
-	if (!retval)
-		retval = fw_map_pages_buf(buf);
-
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
-- 
1.7.9.5

[PATCH 3/4] firmware loader: let direct loading back on 'firmware_buf'

[Ming Lei]

Firstly 'firmware_buf' is introduced to make all loading requests
to share one firmware kernel buffer, so firmware_buf should
be used in direct loading for saving memory and speedup firmware
loading.

Secondly, the commit below

	abb139e75c2cdbb955e840d6331cb5863e409d0e(firmware:teach
	the kernel to load firmware files directly from the filesystem)

introduces direct loading for fixing udev regression, but it
bypasses the firmware cache meachnism, so this patch enables
caching firmware for direct loading case since it is still needed
to solve drivers' dependency during system resume.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |  171 ++++++++++++++++++++++++-----------------
 1 file changed, 100 insertions(+), 71 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f288251..a095d84 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -36,68 +36,6 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
-static const char *fw_path[] = {
-	"/lib/firmware/updates/" UTS_RELEASE,
-	"/lib/firmware/updates",
-	"/lib/firmware/" UTS_RELEASE,
-	"/lib/firmware"
-};
-
-/* Don't inline this: 'struct kstat' is biggish */
-static noinline long fw_file_size(struct file *file)
-{
-	struct kstat st;
-	if (vfs_getattr(file->f_path.mnt, file->f_path.dentry, &st))
-		return -1;
-	if (!S_ISREG(st.mode))
-		return -1;
-	if (st.size != (long)st.size)
-		return -1;
-	return st.size;
-}
-
-static bool fw_read_file_contents(struct file *file, struct firmware *fw)
-{
-	long size;
-	char *buf;
-
-	size = fw_file_size(file);
-	if (size < 0)
-		return false;
-	buf = vmalloc(size);
-	if (!buf)
-		return false;
-	if (kernel_read(file, 0, buf, size) != size) {
-		vfree(buf);
-		return false;
-	}
-	fw->data = buf;
-	fw->size = size;
-	return true;
-}
-
-static bool fw_get_filesystem_firmware(struct firmware *fw, const char *name)
-{
-	int i;
-	bool success = false;
-	char *path = __getname();
-
-	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-		struct file *file;
-		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], name);
-
-		file = filp_open(path, O_RDONLY, 0);
-		if (IS_ERR(file))
-			continue;
-		success = fw_read_file_contents(file, fw);
-		fput(file);
-		if (success)
-			break;
-	}
-	__putname(path);
-	return success;
-}
-
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -150,6 +88,11 @@ enum {
 	FW_STATUS_ABORT,
 };
 
+enum fw_buf_fmt {
+	VMALLOC_BUF,	/* used in direct loading */
+	PAGE_BUF,	/* used in loading via userspace */
+};
+
 static int loading_timeout = 60;	/* In seconds */
 
 static inline long firmware_loading_timeout(void)
@@ -187,6 +130,7 @@ struct firmware_buf {
 	struct completion completion;
 	struct firmware_cache *fwc;
 	unsigned long status;
+	enum fw_buf_fmt fmt;
 	void *data;
 	size_t size;
 	struct page **pages;
@@ -240,6 +184,7 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	strcpy(buf->fw_id, fw_name);
 	buf->fwc = fwc;
 	init_completion(&buf->completion);
+	buf->fmt = VMALLOC_BUF;
 
 	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
 
@@ -307,10 +252,14 @@ static void __fw_free_buf(struct kref *ref)
 	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);
+
+	if (buf->fmt == PAGE_BUF) {
+		vunmap(buf->data);
+		for (i = 0; i < buf->nr_pages; i++)
+			__free_page(buf->pages[i]);
+		kfree(buf->pages);
+	} else
+		vfree(buf->data);
 	kfree(buf);
 }
 
@@ -319,6 +268,69 @@ static void fw_free_buf(struct firmware_buf *buf)
 	kref_put(&buf->ref, __fw_free_buf);
 }
 
+/* direct firmware loading support */
+static const char *fw_path[] = {
+	"/lib/firmware/updates/" UTS_RELEASE,
+	"/lib/firmware/updates",
+	"/lib/firmware/" UTS_RELEASE,
+	"/lib/firmware"
+};
+
+/* Don't inline this: 'struct kstat' is biggish */
+static noinline long fw_file_size(struct file *file)
+{
+	struct kstat st;
+	if (vfs_getattr(file->f_path.mnt, file->f_path.dentry, &st))
+		return -1;
+	if (!S_ISREG(st.mode))
+		return -1;
+	if (st.size != (long)st.size)
+		return -1;
+	return st.size;
+}
+
+static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
+{
+	long size;
+	char *buf;
+
+	size = fw_file_size(file);
+	if (size < 0)
+		return false;
+	buf = vmalloc(size);
+	if (!buf)
+		return false;
+	if (kernel_read(file, 0, buf, size) != size) {
+		vfree(buf);
+		return false;
+	}
+	fw_buf->data = buf;
+	fw_buf->size = size;
+	return true;
+}
+
+static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
+{
+	int i;
+	bool success = false;
+	char *path = __getname();
+
+	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+		struct file *file;
+		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
+
+		file = filp_open(path, O_RDONLY, 0);
+		if (IS_ERR(file))
+			continue;
+		success = fw_read_file_contents(file, buf);
+		fput(file);
+		if (success)
+			break;
+	}
+	__putname(path);
+	return success;
+}
+
 static struct firmware_priv *to_firmware_priv(struct device *dev)
 {
 	return container_of(dev, struct firmware_priv, dev);
@@ -427,6 +439,9 @@ static void firmware_free_data(const struct firmware *fw)
 /* one pages buffer should be mapped/unmapped only once */
 static int fw_map_pages_buf(struct firmware_buf *buf)
 {
+	if (buf->fmt != PAGE_BUF)
+		return 0;
+
 	if (buf->data)
 		vunmap(buf->data);
 	buf->data = vmap(buf->pages, buf->nr_pages, 0, PAGE_KERNEL_RO);
@@ -789,11 +804,6 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
 		return NULL;
 	}
 
-	if (fw_get_filesystem_firmware(firmware, name)) {
-		dev_dbg(device, "firmware: direct-loading firmware %s\n", name);
-		return NULL;
-	}
-
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
 	if (!ret)
 		fw_priv = fw_create_instance(firmware, name, device,
@@ -843,6 +853,21 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	struct device *f_dev = &fw_priv->dev;
 	struct firmware_buf *buf = fw_priv->buf;
 	struct firmware_cache *fwc = &fw_cache;
+	int direct_load = 0;
+
+	/* try direct loading from fs first */
+	if (fw_get_filesystem_firmware(buf)) {
+		dev_dbg(f_dev->parent, "firmware: direct-loading"
+			" firmware %s\n", buf->fw_id);
+
+		set_bit(FW_STATUS_DONE, &buf->status);
+		complete_all(&buf->completion);
+		direct_load = 1;
+		goto handle_fw;
+	}
+
+	/* fall back on userspace loading */
+	buf->fmt = PAGE_BUF;
 
 	dev_set_uevent_suppress(f_dev, true);
 
@@ -881,6 +906,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	del_timer_sync(&fw_priv->timeout);
 
+handle_fw:
 	mutex_lock(&fw_lock);
 	if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status))
 		retval = -ENOENT;
@@ -910,6 +936,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 	fw_priv->buf = NULL;
 	mutex_unlock(&fw_lock);
 
+	if (direct_load)
+		goto err_put_dev;
+
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
 	device_remove_bin_file(f_dev, &firmware_attr_data);
-- 
1.7.9.5

[PATCH 4/4] firmware loader: sync firmware cache by async_synchronize_full_domain

[Ming Lei]

async.c has provided synchronization mechanism on async_schedule_*,
so use async_synchronize_full_domain to sync caching firmware instead
of reinventing the wheel.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a095d84..8945f4e 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -116,8 +116,6 @@ struct firmware_cache {
 	spinlock_t name_lock;
 	struct list_head fw_names;
 
-	wait_queue_head_t wait_queue;
-	int cnt;
 	struct delayed_work work;
 
 	struct notifier_block   pm_notify;
@@ -1166,6 +1164,8 @@ int uncache_firmware(const char *fw_name)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
+
 static struct fw_cache_entry *alloc_fw_cache_entry(const char *name)
 {
 	struct fw_cache_entry *fce;
@@ -1232,12 +1232,6 @@ static void __async_dev_cache_fw_image(void *fw_entry,
 
 		free_fw_cache_entry(fce);
 	}
-
-	spin_lock(&fwc->name_lock);
-	fwc->cnt--;
-	spin_unlock(&fwc->name_lock);
-
-	wake_up(&fwc->wait_queue);
 }
 
 /* called with dev->devres_lock held */
@@ -1278,7 +1272,6 @@ static void dev_cache_fw_image(struct device *dev, void *data)
 		spin_lock(&fwc->name_lock);
 		/* only one cache entry for one firmware */
 		if (!__fw_entry_found(fce->name)) {
-			fwc->cnt++;
 			list_add(&fce->list, &fwc->fw_names);
 		} else {
 			free_fw_cache_entry(fce);
@@ -1287,8 +1280,9 @@ static void dev_cache_fw_image(struct device *dev, void *data)
 		spin_unlock(&fwc->name_lock);
 
 		if (fce)
-			async_schedule(__async_dev_cache_fw_image,
-				       (void *)fce);
+			async_schedule_domain(__async_dev_cache_fw_image,
+					      (void *)fce,
+					      &fw_cache_domain);
 	}
 }
 
@@ -1350,21 +1344,7 @@ static void device_cache_fw_images(void)
 	mutex_unlock(&fw_lock);
 
 	/* 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);
+	async_synchronize_full_domain(&fw_cache_domain);
 
 	loading_timeout = old_timeout;
 }
@@ -1452,9 +1432,7 @@ static void __init fw_cache_init(void)
 #ifdef CONFIG_PM_SLEEP
 	spin_lock_init(&fw_cache.name_lock);
 	INIT_LIST_HEAD(&fw_cache.fw_names);
-	fw_cache.cnt = 0;
 
-	init_waitqueue_head(&fw_cache.wait_queue);
 	INIT_DELAYED_WORK(&fw_cache.work,
 			  device_uncache_fw_images_work);
 
-- 
1.7.9.5

Re: [PATCH 0/4] firmware loader: misc changes

[Ming Lei]

On Tue, Oct 9, 2012 at 12:01 PM, Ming Lei <ming.lei@canonical.com> wrote:
> Hi,
>
> The first two are fixes' patch, the 3rd one is to enable
> caching firmware for direct loading, and the last one is
> a cleanup patch.
>
>  drivers/base/firmware_class.c |  266 +++++++++++++++++++++++------------------
>  1 file changed, 151 insertions(+), 115 deletions(-)

Hi Greg,

Could you push the patchset or at least the first 3 fix patches
onto 3.7-rc?

BTW: 3/4 is still a fix, since linus's direct loading breaks firmware cache
which has been used by some drivers(such as: ath9k)


Thanks,
--
Ming Lei

Re: [PATCH 0/4] firmware loader: misc changes

[Greg Kroah-Hartman]

On Sun, Oct 21, 2012 at 03:55:45PM +0800, Ming Lei wrote:
> On Tue, Oct 9, 2012 at 12:01 PM, Ming Lei <ming.lei@canonical.com> wrote:
> > Hi,
> >
> > The first two are fixes' patch, the 3rd one is to enable
> > caching firmware for direct loading, and the last one is
> > a cleanup patch.
> >
> >  drivers/base/firmware_class.c |  266 +++++++++++++++++++++++------------------
> >  1 file changed, 151 insertions(+), 115 deletions(-)
> 
> Hi Greg,
> 
> Could you push the patchset or at least the first 3 fix patches
> onto 3.7-rc?
> 
> BTW: 3/4 is still a fix, since linus's direct loading breaks firmware cache
> which has been used by some drivers(such as: ath9k)

Yes, sorry, they are still in my queue, I was working on other fixes
for other subsystems this past week.  I'll get to these tomorrow.

greg k-h

Re: [PATCH 0/4] firmware loader: misc changes

[Ming Lei]

On Mon, Oct 22, 2012 at 12:14 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Yes, sorry, they are still in my queue, I was working on other fixes
> for other subsystems this past week.  I'll get to these tomorrow.

No problem, thanks for your work, :-)

Thank,
--
Ming Lei

Re: [PATCH 0/4] firmware loader: misc changes

[Greg Kroah-Hartman]

On Mon, Oct 22, 2012 at 10:54:04AM +0800, Ming Lei wrote:
> On Mon, Oct 22, 2012 at 12:14 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Yes, sorry, they are still in my queue, I was working on other fixes
> > for other subsystems this past week.  I'll get to these tomorrow.
> 
> No problem, thanks for your work, :-)

All applied now, thanks.

greg k-h