[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@xxxxxxxxxxxxx>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/