[PATCH v3 0/5] firmware_class: extensible firmware API

[PATCH v3 0/5] firmware_class: extensible firmware API

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This v3 builds up on the last v2 series from October [0], I had not send
out any updates after that as we had the kernel summit and figured it'd
be best to hash out any kinks there. This patch set *only* provides a new
set of extensible firmware helpers with strong semantics and which avoids
the usermode helper completely. Support for firmware signing was discussed
and we seem to have come to an agreement that it is needed, and how we'd
handle certificates. This patch set does not address that -- that will be
handled in another patch set. If you're curious to take a peak at how that
will be handled though please have a look at the firmware enhancements wiki
page [1], it provides Andy's latest recommendations.

Changes since last v2 (thanks to Josh Boyer and Johannes Berg for feedback):

  * remove WARN_ON() and BUG_ON() - josh
  * make union sysdata_file_cbs const - johannes
  * refer to wiki on future enhancements page
  * upon request by gregkh added myself as maintainer

Please note, I will be actually leaving on vacation far away from computers
tomorrow, so posting this as it was overdue, I just had not had time to get
to this yet. I'll be back January 13th. These patches are based on top of
linux-next tag next-20151223.

[0] http://lkml.kernel.org/r/1443721449-22882-1-git-send-email-mcgrof@do-not-panic.com
[1] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements

David Howells (2):
  firmware: fold successful fw read early
  firmware: generalize reading file contents as a helper

Luis R. Rodriguez (3):
  firmware: generalize "firmware" as "system data" helpers
  firmware: move completing fw into a helper
  firmware: add an extensible system data helpers

 Documentation/firmware_class/system_data.txt |  79 ++++++
 MAINTAINERS                                  |   4 +-
 drivers/base/firmware_class.c                | 385 ++++++++++++++++++++++++---
 include/linux/sysdata.h                      | 212 +++++++++++++++
 4 files changed, 642 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

-- 
2.6.2

[PATCH v3 1/5] firmware: generalize "firmware" as "system data" helpers

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.

Annotate this before we extend firmare_class more,
as this is expected. We want to generalize the code
as much as possible.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vojtěch Pavlík <vojtech@suse.cz>
Cc: Kyle McMartin <kyle@kernel.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/firmware_class.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450e75bd..6f5fcda71a60 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device *device,
 		rc = fw_read_file_contents(file, buf);
 		fput(file);
 		if (rc)
-			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
-				path, rc);
+			dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
+				 path, rc);
 		else
 			break;
 	}
 	__putname(path);
 
 	if (!rc) {
-		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+		dev_dbg(device, "system data: direct-loading firmware %s\n",
 			buf->fw_id);
 		mutex_lock(&fw_lock);
 		set_bit(FW_STATUS_DONE, &buf->status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
-		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+		dev_dbg(device, "system data: using built-in system data%s\n", name);
 		return 0; /* assigned */
 	}
 
-- 
2.6.2

[PATCH v3 2/5] firmware: move completing fw into a helper

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This will be re-used later through a new extensible interface.

Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/firmware_class.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6f5fcda71a60..d8148aa89b01 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,6 +322,15 @@ fail:
 	return rc;
 }
 
+static void fw_finish_direct_load(struct device *device,
+				  struct firmware_buf *buf)
+{
+	mutex_lock(&fw_lock);
+	set_bit(FW_STATUS_DONE, &buf->status);
+	complete_all(&buf->completion);
+	mutex_unlock(&fw_lock);
+}
+
 static int fw_get_filesystem_firmware(struct device *device,
 				       struct firmware_buf *buf)
 {
@@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device *device,
 	if (!rc) {
 		dev_dbg(device, "system data: direct-loading firmware %s\n",
 			buf->fw_id);
-		mutex_lock(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		complete_all(&buf->completion);
-		mutex_unlock(&fw_lock);
+		fw_finish_direct_load(device, buf);
 	}
 
 	return rc;
-- 
2.6.2

[PATCH v3 3/5] firmware: fold successful fw read early

[Luis R. Rodriguez]

From: David Howells <dhowells@redhat.com>

We'll be folding in some more checks on fw_read_file_contents(),
this will make the success case easier to follow.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/firmware_class.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d8148aa89b01..e10a5349aa61 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device *device,
 			continue;
 		rc = fw_read_file_contents(file, buf);
 		fput(file);
-		if (rc)
+		if (rc == 0) {
+			dev_dbg(device, "system data: direct-loading firmware %s\n",
+				buf->fw_id);
+			fw_finish_direct_load(device, buf);
+			goto out;
+		} else
 			dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
 				 path, rc);
-		else
-			break;
 	}
+out:
 	__putname(path);
 
-	if (!rc) {
-		dev_dbg(device, "system data: direct-loading firmware %s\n",
-			buf->fw_id);
-		fw_finish_direct_load(device, buf);
-	}
-
 	return rc;
 }
 
-- 
2.6.2

[PATCH v3 4/5] firmware: generalize reading file contents as a helper

[Luis R. Rodriguez]

From: David Howells <dhowells@redhat.com>

We'll want to reuse this same code later in order to read
two separate types of file contents. This generalizes
fw_read_file_contents() for reading a file and rebrands it
as fw_read_file(). This new caller is now generic: the path
used can be arbitrary and the caller is also agnostic to the
firmware_class code now, this begs the possibility of code
re-use with other similar callers in the kernel. For instance
in the future we may want to share a solution with:

    - firmware_class: fw_read_file()
    - module: kernel_read()
    - kexec: copy_file_fd()

While at it this also cleans up the exit paths on fw_read_file().

Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/firmware_class.c | 62 +++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e10a5349aa61..cd51a90ed012 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -291,34 +291,51 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
+/*
+ * Read the contents of a file.
+ */
+static int fw_read_file(const char *path, void **_buf, size_t *_size)
 {
-	int size;
+	struct file *file;
+	size_t size;
 	char *buf;
 	int rc;
 
+	file = filp_open(path, O_RDONLY, 0);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	rc = -EINVAL;
 	if (!S_ISREG(file_inode(file)->i_mode))
-		return -EINVAL;
+		goto err_file;
 	size = i_size_read(file_inode(file));
 	if (size <= 0)
-		return -EINVAL;
+		goto err_file;
+	rc = -ENOMEM;
 	buf = vmalloc(size);
 	if (!buf)
-		return -ENOMEM;
+		goto err_file;
+
 	rc = kernel_read(file, 0, buf, size);
+	if (rc < 0)
+		goto err_buf;
 	if (rc != size) {
-		if (rc > 0)
-			rc = -EIO;
-		goto fail;
+		rc = -EIO;
+		goto err_buf;
 	}
+
 	rc = security_kernel_fw_from_file(file, buf, size);
 	if (rc)
-		goto fail;
-	fw_buf->data = buf;
-	fw_buf->size = size;
+		goto err_buf;
+
+	*_buf = buf;
+	*_size = size;
 	return 0;
-fail:
+
+err_buf:
 	vfree(buf);
+err_file:
+	fput(file);
 	return rc;
 }
 
@@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
 }
 
 static int fw_get_filesystem_firmware(struct device *device,
-				       struct firmware_buf *buf)
+				      struct firmware_buf *buf)
 {
 	int i, len;
 	int rc = -ENOENT;
-	char *path;
+	char *path = NULL;
 
 	path = __getname();
 	if (!path)
 		return -ENOMEM;
 
+	/*
+	 * Try each possible firmware blob in turn till one doesn't produce
+	 * ENOENT.
+	 */
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-		struct file *file;
-
 		/* skip the unset customized path */
 		if (!fw_path[i][0])
 			continue;
@@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device *device,
 			break;
 		}
 
-		file = filp_open(path, O_RDONLY, 0);
-		if (IS_ERR(file))
-			continue;
-		rc = fw_read_file_contents(file, buf);
-		fput(file);
+		rc = fw_read_file(path, &buf->data, &buf->size);
 		if (rc == 0) {
 			dev_dbg(device, "system data: direct-loading firmware %s\n",
 				buf->fw_id);
 			fw_finish_direct_load(device, buf);
 			goto out;
-		} else
+		} else if (rc != -ENOENT) {
 			dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
 				 path, rc);
+			goto out;
+		}
 	}
 out:
 	__putname(path);
-
 	return rc;
 }
 
-- 
2.6.2

[PATCH v3 5/5] firmware: add an extensible system data helpers

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

The firmware API has evolved over the years slowly, as it
grows we extend it by adding new routines or at times we extend
existing routines with more or less arguments. This doesn't scale
well, when new arguments are added to existing routines it means
we need to traverse the kernel with a slew of collateral
evolutions to adjust old driver users. The firmware API is also
now being used for things outside of the scope of what typically
would be considered "firmware", an example here is the p54 driver
enables users to provide a custom EEPROM through this interface.
Another example is optional CPU microcode updates.

There are other subsystems which would like to make use of the
APIs for similar things but have different requirements and
criteria which they'd like to be met for the requested file.
If different requirements are needed it would again mean adding
more arguments and making a slew of collateral evolutions, or
adding yet-another-new-API-call.

Instead of extending the existing firmware API even more this
provides an new extensible API which can be used to supercede the
old firmware API without causing a series of collateral evolutions
as requirements grow. This leaves the old firmware API as-is,
ignores all consideration for usermode-helpers, labels the new API
to reflect its broad use outside of the scope of firmware: system
data helpers, and builds on top of the original firmware core code.

The new extensible "system data" set of helpers accepts that there
really are only two types of requests for accessing system data:

a) synchronous requests
b) asynchronous requests

Both of these requests may have a different set of requirements
which must be met. These requirements can simply be passed as a
descriptors to each type of request. The descriptor can be extended
over time to support different requirements as the kernel evolves.

Using the new system data helpers is only necessary if you have
requirements outside of what the existing old firmware API accepts.
We encourage developers to leave the old API as-is and extend the
new descriptors and system data code to provide support for new
features.

A few simple features added as part of the new set of system data
request APIs, other than making the new API easily extensible for
the future:

 - By default the kernel will free the system data file for you after
   your callbacks are called, you however are allowed to request to that
   you wish to keep the system data file on the descriptor. This is
   dealt with by requiring a consumer callback for the system data file.
 - Allow both asynchronous and synchronous request to specify that system data
   files are optional. With the old APIs we had added one full API call,
   request_firmware_direct() just for this purpose -- although it should be
   noted another of its goal was to also skip the usermode helper.
   The system data request APIs allow for you to annotate that a system
   data file is optional for both synchronous or asynchronous requests
   through the same two basic set of APIs.
 - Usermode helpers are completely ignored, always
 - The system data request APIs currently match the old synchronous firmware
   API calls to refcounted firmware_class module, but it should be easy
   to add support now to enable also refcounting the caller's module
   should it be be needed. Likewise the system data request APIs match the
   old asynchronous firmware API call and refcounts the caller's module.

In order to try to help phase out user mode helpers this makes no use of
the old user mode helper code *at all*, and if we wish to can easily
phase this code out with time then.

Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 Documentation/firmware_class/system_data.txt |  79 ++++++++
 MAINTAINERS                                  |   4 +-
 drivers/base/firmware_class.c                | 291 +++++++++++++++++++++++++++
 include/linux/sysdata.h                      | 212 +++++++++++++++++++
 4 files changed, 585 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

diff --git a/Documentation/firmware_class/system_data.txt b/Documentation/firmware_class/system_data.txt
new file mode 100644
index 000000000000..ab509243455a
--- /dev/null
+++ b/Documentation/firmware_class/system_data.txt
@@ -0,0 +1,79 @@
+System data requests API
+========================
+
+As the kernel evolves we keep extending the firmware_class set of APIs
+with more or less arguments, this creates a slew of collateral evolutions.
+The set of users of firmware request APIs has also grown now to include
+users which are not looking for "firmware" per se, but instead general
+system data files which for one reason or another has been decided to be
+kept oustide of the kernel, and/or to allow dynamic updates. The system data
+request set of APIs addresses rebranding of firmware as generic system data
+files, and provides a way to enable these APIs to easily be extended without
+much collateral evolutions.
+
+System data modes of operation
+==============================
+
+There are only two types of modes of operation for system data requests:
+
+  * synchronous  - sysdata_file_request()
+  * asynchronous - sysdata_file_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+System data file descriptor
+===========================
+
+Variations of types of system data requests are specified by a system  data
+request descriptor. The system data request descriptor can grow as with new
+fields as requirements grow. The old firmware API provides two synchronous
+requests: request_firmware() and request_firmware_direct(), the later allowing
+the caller to specify that the "system data file" is optional. The system data
+request API allows a caller to set the optional nature of the system data file
+on the system data file descriptor using the same synchronous API. Since this
+requirement is part of the descriptor it also allows asynchronous requests
+to specify that the system data file is optional.
+
+Reference counting and releasing the system data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the system data requests. This prevents removal
+of the device and module making the system data request call until the
+system data request callbacks have completed, either synchronously or
+asynchronously.
+
+The old firmware APIs refcounted the firmware_class module for synchronous
+requests, meanwhile asynchronous requests refcounted the caller's module.
+The system data request API currently mimic this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs, although if in the future we may later enable use of
+also refcounting the caller's module as well. Likewise in the future we
+may extend asynchronous calls to refcount the firmware_class module.
+
+Typical use of the old synchronous firmware APIs consist of the caller
+requesting for "system data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "system data" and then finally freeing it on
+asynchronous callback.
+
+The system data request API enables callers to provide a callback for both
+synchronous and asynchronous requests and since consumption can be expected
+in these callbacks it frees it for you by default after callback handlers
+are issued. If you wish to keep the system data around after your callbacks
+you must specify this through the system data request descriptor.
+
+User mode helper
+================
+
+The old firmware API provided support for an optional user mode helper. The
+new system data request API abandons all notions of the usermode helper.
+
+Tracking development enhancements and ideas
+===========================================
+
+To help track ongoing development for firmware_class and related items to
+firmware_class refer to the kernel newbies wiki page [0].
+
+[0] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements
diff --git a/MAINTAINERS b/MAINTAINERS
index 44666b126bc7..3a7d5b7543dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4406,13 +4406,15 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, sysdata_file_request)
 M:	Ming Lei <ming.lei@canonical.com>
+M:	Luis R. Rodriguez <mcgrof@kernel.org>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/sysdata.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <josh.h.morris@us.ibm.com>
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index cd51a90ed012..baf62978d2d0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/highmem.h>
+#include <linux/sysdata.h>
 #include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -38,6 +39,12 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+static const struct sysdata_file_sync_reqs dfl_sync_reqs = {
+	.mode = SYNCDATA_SYNC,
+	.module = THIS_MODULE,
+	.gfp = GFP_KERNEL,
+};
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -1272,6 +1279,182 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static void sysdata_file_update(struct sysdata_file *sysdata)
+{
+	struct firmware *fw;
+	struct firmware_buf *buf;
+
+	if (!sysdata || !sysdata->priv)
+		return;
+
+	fw = sysdata->priv;
+	if (!fw->priv)
+		return;
+
+	buf = fw->priv;
+
+	sysdata->size = buf->size;
+	sysdata->data = buf->data;
+
+	pr_debug("%s: fw-%s buf=%p data=%p size=%u",
+		 __func__, buf->fw_id, buf, buf->data,
+		 (unsigned int)buf->size);
+}
+
+/*
+ * prepare firmware and firmware_buf structs;
+ * return 0 if a firmware is already assigned, 1 if need to load one,
+ * or a negative error code
+ */
+static int
+_request_sysdata_prepare(struct sysdata_file **sysdata_p, const char *name,
+			  struct device *device)
+{
+	struct sysdata_file *sysdata;
+	struct firmware *fw;
+	int ret;
+
+	*sysdata_p = sysdata = kzalloc(sizeof(*sysdata), GFP_KERNEL);
+	if (!sysdata) {
+		dev_err(device, "%s: kmalloc(struct sysdata) failed\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ret = _request_firmware_prepare(&fw, name, device);
+	if (ret >= 0)
+		sysdata->priv = fw;
+
+	return ret;
+}
+
+/**
+ * release_sysdata_file: - release the resource associated with the sysdata file
+ * @sysdata_file: sysdata resource to release
+ **/
+void release_sysdata_file(const struct sysdata_file *sysdata)
+{
+	struct firmware *fw;
+
+	if (sysdata) {
+		if (sysdata->priv) {
+			fw = sysdata->priv;
+			release_firmware(fw);
+		}
+	}
+	kfree(sysdata);
+}
+EXPORT_SYMBOL_GPL(release_sysdata_file);
+
+/*
+ * sysdata_p is always set to be NULL unless a proper system
+ * data file was found.
+ */
+static int _sysdata_file_request(const struct sysdata_file **sysdata_p,
+				 const char *name,
+				 const struct sysdata_file_desc *desc,
+				 struct device *device)
+{
+	struct sysdata_file *sysdata = NULL;
+	struct firmware *fw = NULL;
+	int ret = -EINVAL;
+
+	if (!sysdata_p)
+		goto out;
+
+	if (!desc)
+		goto out;
+
+	if (!name || name[0] == '\0')
+		goto out;
+
+	ret = _request_sysdata_prepare(&sysdata, name, device);
+	if (ret <= 0) /* error or already assigned */
+		goto out;
+
+	fw = sysdata->priv;
+
+	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (ret && !desc->optional)
+		pr_err("Direct system data load for %s failed with error %d\n",
+		       name, ret);
+
+	if (!ret)
+		ret = assign_firmware_buf(fw, device, FW_OPT_UEVENT);
+
+ out:
+	if (ret < 0) {
+		release_sysdata_file(sysdata);
+		sysdata = NULL;
+	}
+
+	sysdata_file_update(sysdata);
+
+	*sysdata_p = sysdata;
+
+	return ret;
+}
+
+/**
+ * sysdata_file_request - synchronous request for a system data file
+ * @name: name of the system data file
+ * @desc: system data file descriptor, it provides all the requirements
+ * 	which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ *
+ * This performs a synchronous system data file lookup with the requirements
+ * specified on @desc, if the file was found meeting the criteria requested
+ * 0 is returned. Access to the system data file data can be accessed through
+ * an optional callback set on the @desc. If the system data file is optional
+ * you must specify that on the @desc and if set you may provide an alternative
+ * callback which if set would be run if the system data file was not found.
+ *
+ * The system data file passed to the callbacks will always be NULL unless
+ * the it was found matching all the criteria on @desc. 0 is always returned
+ * if the file was found unless a callback was provided, in which case the
+ * callback's return value will be passed. Unless the desc->keep was set the
+ * kernel will release the system data file for you after your callbacks
+ * were processed.
+ *
+ * Reference counting is used during the duration of this call on both the
+ * device and module that made the request. This prevents any callers from
+ * freeing either the device or module prior to completion of this call.
+ */
+int sysdata_file_request(const char *name,
+			 const struct sysdata_file_desc *desc,
+			 struct device *device)
+{
+	const struct sysdata_file *sysdata;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+	int ret;
+
+	if (!device || !desc || !name)
+		return -EINVAL;
+
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+
+	sync_reqs = &dfl_sync_reqs;
+
+	__module_get(sync_reqs->module);
+	get_device(device);
+
+	ret = _sysdata_file_request(&sysdata, name, desc, device);
+	if (ret && desc->optional)
+		ret = desc_sync_opt_call_cb(desc);
+	else
+		ret = desc_sync_found_call_cb(desc, sysdata);
+
+	if (!desc->keep)
+		release_sysdata_file(sysdata);
+
+	put_device(device);
+	module_put(sync_reqs->module);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sysdata_file_request);
+
 /* Async support */
 struct firmware_work {
 	struct work_struct work;
@@ -1360,6 +1543,114 @@ request_firmware_nowait(
 }
 EXPORT_SYMBOL(request_firmware_nowait);
 
+struct sysdata_file_work {
+	struct work_struct work;
+	const char *name;
+	struct sysdata_file_desc desc;
+	struct device *device;
+};
+
+static void request_sysdata_file_work_func(struct work_struct *work)
+{
+	struct sysdata_file_work *sys_work;
+	const struct sysdata_file_desc *desc;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+	const struct sysdata_file *sysdata;
+	int ret;
+
+	sys_work = container_of(work, struct sysdata_file_work, work);
+	desc = &sys_work->desc;
+	sync_reqs = &desc->sync_reqs;
+
+	ret = _sysdata_file_request(&sysdata, sys_work->name,
+				    desc, sys_work->device);
+	if (ret && desc->optional)
+		desc_async_opt_call_cb(desc);
+	else
+		desc_async_found_call_cb(sysdata, desc);
+
+	if (!desc->keep)
+		release_sysdata_file(sysdata);
+
+	put_device(sys_work->device);
+	module_put(sync_reqs->module);
+
+	kfree_const(sys_work->name);
+	kfree(sys_work);
+}
+
+/**
+ * sysdata_file_request_async - asynchronous request for a system data file
+ * @name: name of the system data file
+ * @desc: system data file descriptor, it provides all the requirements
+ * 	which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ *
+ * This performs an asynchronous system data file lookup with the requirements
+ * specified on @desc. The request for the actual system data file lookup will
+ * be scheduled with schedule_work() to be run at a later time. 0 is returned
+ * if we were able to schedlue the work to be run.
+ *
+ * Reference counting is used during the duration of this scheduled call on
+ * both the device and module that made the request. This prevents any callers
+ * from freeing either the device or module prior to completion of the
+ * scheduled work.
+ *
+ * Access to the system data file data can be accessed through an optional
+ * callback set on the @desc. If the system data file is optional you must
+ * specify that on the @desc and if set you may provide an alternative
+ * callback which if set would be run if the system data file was not found.
+ *
+ * The system data file passed to the callbacks will always be NULL unless
+ * the it was found matching all the criteria on @desc. Unless the desc->keep
+ * was set the kernel will release the system data file for you after your
+ * callbacks were processed on the scheduled work.
+ *
+ */
+int sysdata_file_request_async(const char *name,
+			       const struct sysdata_file_desc *desc,
+			       struct device *device)
+{
+	struct sysdata_file_work *sys_work;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+
+	if (!device || !desc || !name)
+		return -EINVAL;
+
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return -EINVAL;
+
+	sync_reqs = &desc->sync_reqs;
+
+	if (!sync_reqs->module)
+		return -EINVAL;
+
+	sys_work = kzalloc(sizeof(struct sysdata_file_work), sync_reqs->gfp);
+	if (!sys_work)
+		return -ENOMEM;
+
+	sys_work->device = device;
+	memcpy(&sys_work->desc, desc, sizeof(struct sysdata_file_desc));
+	sys_work->name = kstrdup_const(name, sync_reqs->gfp);
+	if (!sys_work->name) {
+		kfree(sys_work);
+		return -ENOMEM;
+	}
+
+	if (!try_module_get(sync_reqs->module)) {
+		kfree_const(sys_work->name);
+		kfree(sys_work);
+		return -EFAULT;
+	}
+
+	get_device(sys_work->device);
+	INIT_WORK(&sys_work->work, request_sysdata_file_work_func);
+	schedule_work(&sys_work->work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sysdata_file_request_async);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
new file mode 100644
index 000000000000..b4fdd941ee27
--- /dev/null
+++ b/include/linux/sysdata.h
@@ -0,0 +1,212 @@
+#ifndef _LINUX_SYSDATA_H
+#define _LINUX_SYSDATA_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/gfp.h>
+
+/*
+ * System Data internals
+ *
+ * Copyright (C) 2015 Luis R. Rodriguez <mcgrof@do-not-panic.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+struct sysdata_file {
+	size_t size;
+	const u8 *data;
+
+	/* sysdata loader private fields */
+	void *priv;
+};
+
+/**
+ * enum sync_data_mode - system data mode of operation
+ *
+ * SYNCDATA_SYNC: your call to request system data is synchronous. We will
+ * 	look for the system data file you have requested immediatley.
+ * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
+ * 	schedule the search for your system data file to be run at a later
+ * 	time.
+ */
+enum sync_data_mode {
+	SYNCDATA_SYNC,
+	SYNCDATA_ASYNC,
+};
+
+/* one per sync_data_mode */
+union sysdata_file_cbs {
+	struct {
+		int __must_check (*found_cb)(void *, const struct sysdata_file *);
+		void *found_context;
+
+		int __must_check (*opt_fail_cb)(void *);
+		void *opt_fail_context;
+	} sync;
+	struct {
+		void (*found_cb)(const struct sysdata_file *, void *);
+		void *found_context;
+
+		void (*opt_fail_cb)(void *);
+		void *opt_fail_context;
+	} async;
+};
+
+struct sysdata_file_sync_reqs {
+	enum sync_data_mode mode;
+	struct module *module;
+	gfp_t gfp;
+};
+
+/**
+ * struct sysdata_file_desc - system data file descriptor
+ * @optional: if true it is not a hard requirement by the caller that this
+ *	file be present. An error will not be recorded if the file is not
+ *	found.
+ * @keep: if set the caller wants to claim ownership over the system data
+ *	through one of its callbacks, it must later free it with
+ *	release_sysdata_file(). By default this is set to false and the kernel
+ *	will release the system data file for you after callback processing
+ *	has completed.
+ * @sync_reqs: synchronization requirements, this will be taken care for you
+ *	by default if you are usingy sdata_file_request(), otherwise you
+ *	should provide your own requirements.
+ *
+ * This structure is set the by the driver and passed to the system data
+ * file helpers sysdata_file_request() or sysdata_file_request_async().
+ * It is intended to carry all requirements and specifications required
+ * to complete the task to get the requested system date file to the caller.
+ * If you wish to extend functionality of system data file requests you
+ * should extend this data structure and make use of the extensions on
+ * the callers to avoid unnecessary collateral evolutions.
+ *
+ * You are allowed to provide a callback to handle if a system data file was
+ * found or not. You do not need to provide a callback. You may also set
+ * an optional flag which would enable you to declare that the system data
+ * file is optional and that if it is not found an alternative callback be
+ * run for you.
+ *
+ * Refer to sysdata_file_request() and sysdata_file_request_async() for more
+ * details.
+ */
+struct sysdata_file_desc {
+	bool optional;
+	bool keep;
+	struct sysdata_file_sync_reqs sync_reqs;
+	const union sysdata_file_cbs cbs;
+};
+
+/*
+ * We keep these template definitions to a minimum for the most
+ * popular requests.
+ */
+
+/* Typical sync data case */
+#define SYSDATA_SYNC_FOUND(__found_cb, __context)			\
+	.cbs.sync.found_cb = __found_cb,				\
+	.cbs.sync.found_context = __context
+
+/* If you have one fallback routine */
+#define SYSDATA_SYNC_OPT_CB(__found_cb, __context)			\
+	.cbs.sync.opt_fail_cb = __found_cb,				\
+	.cbs.sync.opt_fail_context = __context
+
+/*
+ * Used to define the default asynchronization requirements for
+ * sysdata_file_request_async(). Drivers can override.
+ */
+#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context)			\
+	.sync_reqs = {							\
+		.mode = SYNCDATA_ASYNC,					\
+		.module = THIS_MODULE,					\
+		.gfp = GFP_KERNEL,					\
+	},								\
+	.cbs.async = {							\
+		.found_cb = __found_cb,					\
+		.found_context = __context,				\
+	}
+
+#define desc_sync_found_cb(desc)	((desc)->cbs.sync.found_cb)
+#define desc_sync_found_context(desc)	((desc)->cbs.sync.found_context)
+static inline int desc_sync_found_call_cb(const struct sysdata_file_desc *desc,
+					  const struct sysdata_file *sysdata)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+	if (!desc_sync_found_cb(desc)) {
+		if (sysdata)
+			return 0;
+		return -ENOENT;
+	}
+	return desc_sync_found_cb(desc)(desc_sync_found_context(desc),
+					sysdata);
+}
+
+#define desc_sync_opt_cb(desc)		((desc)->cbs.sync.opt_fail_cb)
+#define desc_sync_opt_context(desc)	((desc)->cbs.sync.opt_fail_context)
+static inline int desc_sync_opt_call_cb(const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+	if (!desc_sync_opt_cb(desc))
+		return 0;
+	return desc_sync_opt_cb(desc)(desc_sync_opt_context(desc));
+}
+
+#define desc_async_found_cb(desc)	((desc)->cbs.async.found_cb)
+#define desc_async_found_context(desc)	((desc)->cbs.async.found_context)
+static inline void desc_async_found_call_cb(const struct sysdata_file *sysdata,
+					    const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return;
+	if (!desc_async_found_cb(desc))
+		return;
+	desc_async_found_cb(desc)(sysdata, desc_async_found_context(desc));
+}
+
+#define desc_async_opt_cb(desc)		((desc)->cbs.async.opt_fail_cb)
+#define desc_async_opt_context(desc)	((desc)->cbs.async.opt_fail_context)
+static inline void desc_async_opt_call_cb(const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return;
+	if (!desc_async_opt_cb(desc))
+		return;
+	desc_async_opt_cb(desc)(desc_async_opt_context(desc));
+}
+
+
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+int sysdata_file_request(const char *name,
+			 const struct sysdata_file_desc *desc,
+			 struct device *device);
+int sysdata_file_request_async(const char *name,
+			       const struct sysdata_file_desc *desc,
+			       struct device *device);
+void release_sysdata_file(const struct sysdata_file *sysdata);
+#else
+static inline int sysdata_file_request(const char *name,
+				       const struct sysdata_file_desc *desc,
+				       struct device *device)
+{
+	return -EINVAL;
+}
+
+static inline int sysdata_file_request_async(const char *name,
+					     const struct sysdata_file_desc *desc,
+					     struct device *device);
+{
+	return -EINVAL;
+}
+
+static inline void release_sysdata_file(const struct sysdata_file *sysdata)
+{
+}
+#endif
+
+#endif /* _LINUX_SYSDATA_H */
-- 
2.6.2

Re: [PATCH v3 5/5] firmware: add an extensible system data helpers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

Hi Luis,

[auto build test WARNING on v4.4-rc6]
[also build test WARNING on next-20151223]

url:    https://github.com/0day-ci/linux/commits/Luis-R-Rodriguez/firmware_class-extensible-firmware-API/20151224-053852
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/init.h:1: warning: no structured comments found
   kernel/sys.c:1: warning: no structured comments found
>> drivers/base/firmware_class.c:1336: warning: No description found for parameter 'sysdata'
>> drivers/base/firmware_class.c:1336: warning: Excess function parameter 'sysdata_file' description in 'release_sysdata_file'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   drivers/dma-buf/reservation.c:1: warning: no structured comments found
   include/linux/reservation.h:1: warning: no structured comments found
   include/linux/hsi/hsi.h:150: warning: Excess struct/union/enum/typedef member 'e_handler' description in 'hsi_client'
   include/linux/hsi/hsi.h:150: warning: Excess struct/union/enum/typedef member 'pclaimed' description in 'hsi_client'
   include/linux/hsi/hsi.h:150: warning: Excess struct/union/enum/typedef member 'nb' description in 'hsi_client'

vim +/sysdata +1336 drivers/base/firmware_class.c

  1320				__func__);
  1321			return -ENOMEM;
  1322		}
  1323	
  1324		ret = _request_firmware_prepare(&fw, name, device);
  1325		if (ret >= 0)
  1326			sysdata->priv = fw;
  1327	
  1328		return ret;
  1329	}
  1330	
  1331	/**
  1332	 * release_sysdata_file: - release the resource associated with the sysdata file
  1333	 * @sysdata_file: sysdata resource to release
  1334	 **/
  1335	void release_sysdata_file(const struct sysdata_file *sysdata)
> 1336	{
  1337		struct firmware *fw;
  1338	
  1339		if (sysdata) {
  1340			if (sysdata->priv) {
  1341				fw = sysdata->priv;
  1342				release_firmware(fw);
  1343			}
  1344		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6124 bytes --]

Re: [PATCH v3 5/5] firmware: add an extensible system data helpers

[Kees Cook]

On Wed, Dec 23, 2015 at 1:34 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> The firmware API has evolved over the years slowly, as it
> grows we extend it by adding new routines or at times we extend
> existing routines with more or less arguments. This doesn't scale
> well, when new arguments are added to existing routines it means
> we need to traverse the kernel with a slew of collateral
> evolutions to adjust old driver users. The firmware API is also
> now being used for things outside of the scope of what typically
> would be considered "firmware", an example here is the p54 driver
> enables users to provide a custom EEPROM through this interface.
> Another example is optional CPU microcode updates.
>
> There are other subsystems which would like to make use of the
> APIs for similar things but have different requirements and
> criteria which they'd like to be met for the requested file.
> If different requirements are needed it would again mean adding
> more arguments and making a slew of collateral evolutions, or
> adding yet-another-new-API-call.
>
> Instead of extending the existing firmware API even more this
> provides an new extensible API which can be used to supercede the
> old firmware API without causing a series of collateral evolutions
> as requirements grow. This leaves the old firmware API as-is,
> ignores all consideration for usermode-helpers, labels the new API
> to reflect its broad use outside of the scope of firmware: system
> data helpers, and builds on top of the original firmware core code.
>
> The new extensible "system data" set of helpers accepts that there
> really are only two types of requests for accessing system data:
>
> a) synchronous requests
> b) asynchronous requests
>
> Both of these requests may have a different set of requirements
> which must be met. These requirements can simply be passed as a
> descriptors to each type of request. The descriptor can be extended
> over time to support different requirements as the kernel evolves.
>
> Using the new system data helpers is only necessary if you have
> requirements outside of what the existing old firmware API accepts.
> We encourage developers to leave the old API as-is and extend the
> new descriptors and system data code to provide support for new
> features.
>
> A few simple features added as part of the new set of system data
> request APIs, other than making the new API easily extensible for
> the future:
>
>  - By default the kernel will free the system data file for you after
>    your callbacks are called, you however are allowed to request to that
>    you wish to keep the system data file on the descriptor. This is
>    dealt with by requiring a consumer callback for the system data file.
>  - Allow both asynchronous and synchronous request to specify that system data
>    files are optional. With the old APIs we had added one full API call,
>    request_firmware_direct() just for this purpose -- although it should be
>    noted another of its goal was to also skip the usermode helper.
>    The system data request APIs allow for you to annotate that a system
>    data file is optional for both synchronous or asynchronous requests
>    through the same two basic set of APIs.
>  - Usermode helpers are completely ignored, always
>  - The system data request APIs currently match the old synchronous firmware
>    API calls to refcounted firmware_class module, but it should be easy
>    to add support now to enable also refcounting the caller's module
>    should it be be needed. Likewise the system data request APIs match the
>    old asynchronous firmware API call and refcounts the caller's module.
>
> In order to try to help phase out user mode helpers this makes no use of
> the old user mode helper code *at all*, and if we wish to can easily
> phase this code out with time then.

So these are basically wrappers around the existing firmware loading routines?

-Kees

>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  Documentation/firmware_class/system_data.txt |  79 ++++++++
>  MAINTAINERS                                  |   4 +-
>  drivers/base/firmware_class.c                | 291 +++++++++++++++++++++++++++
>  include/linux/sysdata.h                      | 212 +++++++++++++++++++
>  4 files changed, 585 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/firmware_class/system_data.txt
>  create mode 100644 include/linux/sysdata.h
>
> diff --git a/Documentation/firmware_class/system_data.txt b/Documentation/firmware_class/system_data.txt
> new file mode 100644
> index 000000000000..ab509243455a
> --- /dev/null
> +++ b/Documentation/firmware_class/system_data.txt
> @@ -0,0 +1,79 @@
> +System data requests API
> +========================
> +
> +As the kernel evolves we keep extending the firmware_class set of APIs
> +with more or less arguments, this creates a slew of collateral evolutions.
> +The set of users of firmware request APIs has also grown now to include
> +users which are not looking for "firmware" per se, but instead general
> +system data files which for one reason or another has been decided to be
> +kept oustide of the kernel, and/or to allow dynamic updates. The system data
> +request set of APIs addresses rebranding of firmware as generic system data
> +files, and provides a way to enable these APIs to easily be extended without
> +much collateral evolutions.
> +
> +System data modes of operation
> +==============================
> +
> +There are only two types of modes of operation for system data requests:
> +
> +  * synchronous  - sysdata_file_request()
> +  * asynchronous - sysdata_file_request_async()
> +
> +Synchronous requests expect requests to be done immediately, asynchronous
> +requests enable requests to be scheduled for a later time.
> +
> +System data file descriptor
> +===========================
> +
> +Variations of types of system data requests are specified by a system  data
> +request descriptor. The system data request descriptor can grow as with new
> +fields as requirements grow. The old firmware API provides two synchronous
> +requests: request_firmware() and request_firmware_direct(), the later allowing
> +the caller to specify that the "system data file" is optional. The system data
> +request API allows a caller to set the optional nature of the system data file
> +on the system data file descriptor using the same synchronous API. Since this
> +requirement is part of the descriptor it also allows asynchronous requests
> +to specify that the system data file is optional.
> +
> +Reference counting and releasing the system data file
> +=====================================================
> +
> +As with the old firmware API both the device and module are bumped with
> +reference counts during the system data requests. This prevents removal
> +of the device and module making the system data request call until the
> +system data request callbacks have completed, either synchronously or
> +asynchronously.
> +
> +The old firmware APIs refcounted the firmware_class module for synchronous
> +requests, meanwhile asynchronous requests refcounted the caller's module.
> +The system data request API currently mimic this behaviour, for synchronous
> +requests the firmware_class module is refcounted through the use of
> +dfl_sync_reqs, although if in the future we may later enable use of
> +also refcounting the caller's module as well. Likewise in the future we
> +may extend asynchronous calls to refcount the firmware_class module.
> +
> +Typical use of the old synchronous firmware APIs consist of the caller
> +requesting for "system data", consuming it after a request and finally
> +freeing it. Typical asynchronous use of the old firmware APIs consist of
> +the caller requesting for "system data" and then finally freeing it on
> +asynchronous callback.
> +
> +The system data request API enables callers to provide a callback for both
> +synchronous and asynchronous requests and since consumption can be expected
> +in these callbacks it frees it for you by default after callback handlers
> +are issued. If you wish to keep the system data around after your callbacks
> +you must specify this through the system data request descriptor.
> +
> +User mode helper
> +================
> +
> +The old firmware API provided support for an optional user mode helper. The
> +new system data request API abandons all notions of the usermode helper.
> +
> +Tracking development enhancements and ideas
> +===========================================
> +
> +To help track ongoing development for firmware_class and related items to
> +firmware_class refer to the kernel newbies wiki page [0].
> +
> +[0] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 44666b126bc7..3a7d5b7543dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4406,13 +4406,15 @@ F:      include/linux/firewire.h
>  F:     include/uapi/linux/firewire*.h
>  F:     tools/firewire/
>
> -FIRMWARE LOADER (request_firmware)
> +FIRMWARE LOADER (request_firmware, sysdata_file_request)
>  M:     Ming Lei <ming.lei@canonical.com>
> +M:     Luis R. Rodriguez <mcgrof@kernel.org>
>  L:     linux-kernel@vger.kernel.org
>  S:     Maintained
>  F:     Documentation/firmware_class/
>  F:     drivers/base/firmware*.c
>  F:     include/linux/firmware.h
> +F:     include/linux/sysdata.h
>
>  FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
>  M:     Joshua Morris <josh.h.morris@us.ibm.com>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index cd51a90ed012..baf62978d2d0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -18,6 +18,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/highmem.h>
> +#include <linux/sysdata.h>
>  #include <linux/firmware.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> @@ -38,6 +39,12 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
>  MODULE_LICENSE("GPL");
>
> +static const struct sysdata_file_sync_reqs dfl_sync_reqs = {
> +       .mode = SYNCDATA_SYNC,
> +       .module = THIS_MODULE,
> +       .gfp = GFP_KERNEL,
> +};
> +
>  /* Builtin firmware support */
>
>  #ifdef CONFIG_FW_LOADER
> @@ -1272,6 +1279,182 @@ void release_firmware(const struct firmware *fw)
>  }
>  EXPORT_SYMBOL(release_firmware);
>
> +static void sysdata_file_update(struct sysdata_file *sysdata)
> +{
> +       struct firmware *fw;
> +       struct firmware_buf *buf;
> +
> +       if (!sysdata || !sysdata->priv)
> +               return;
> +
> +       fw = sysdata->priv;
> +       if (!fw->priv)
> +               return;
> +
> +       buf = fw->priv;
> +
> +       sysdata->size = buf->size;
> +       sysdata->data = buf->data;
> +
> +       pr_debug("%s: fw-%s buf=%p data=%p size=%u",
> +                __func__, buf->fw_id, buf, buf->data,
> +                (unsigned int)buf->size);
> +}
> +
> +/*
> + * prepare firmware and firmware_buf structs;
> + * return 0 if a firmware is already assigned, 1 if need to load one,
> + * or a negative error code
> + */
> +static int
> +_request_sysdata_prepare(struct sysdata_file **sysdata_p, const char *name,
> +                         struct device *device)
> +{
> +       struct sysdata_file *sysdata;
> +       struct firmware *fw;
> +       int ret;
> +
> +       *sysdata_p = sysdata = kzalloc(sizeof(*sysdata), GFP_KERNEL);
> +       if (!sysdata) {
> +               dev_err(device, "%s: kmalloc(struct sysdata) failed\n",
> +                       __func__);
> +               return -ENOMEM;
> +       }
> +
> +       ret = _request_firmware_prepare(&fw, name, device);
> +       if (ret >= 0)
> +               sysdata->priv = fw;
> +
> +       return ret;
> +}
> +
> +/**
> + * release_sysdata_file: - release the resource associated with the sysdata file
> + * @sysdata_file: sysdata resource to release
> + **/
> +void release_sysdata_file(const struct sysdata_file *sysdata)
> +{
> +       struct firmware *fw;
> +
> +       if (sysdata) {
> +               if (sysdata->priv) {
> +                       fw = sysdata->priv;
> +                       release_firmware(fw);
> +               }
> +       }
> +       kfree(sysdata);
> +}
> +EXPORT_SYMBOL_GPL(release_sysdata_file);
> +
> +/*
> + * sysdata_p is always set to be NULL unless a proper system
> + * data file was found.
> + */
> +static int _sysdata_file_request(const struct sysdata_file **sysdata_p,
> +                                const char *name,
> +                                const struct sysdata_file_desc *desc,
> +                                struct device *device)
> +{
> +       struct sysdata_file *sysdata = NULL;
> +       struct firmware *fw = NULL;
> +       int ret = -EINVAL;
> +
> +       if (!sysdata_p)
> +               goto out;
> +
> +       if (!desc)
> +               goto out;
> +
> +       if (!name || name[0] == '\0')
> +               goto out;
> +
> +       ret = _request_sysdata_prepare(&sysdata, name, device);
> +       if (ret <= 0) /* error or already assigned */
> +               goto out;
> +
> +       fw = sysdata->priv;
> +
> +       ret = fw_get_filesystem_firmware(device, fw->priv);
> +       if (ret && !desc->optional)
> +               pr_err("Direct system data load for %s failed with error %d\n",
> +                      name, ret);
> +
> +       if (!ret)
> +               ret = assign_firmware_buf(fw, device, FW_OPT_UEVENT);
> +
> + out:
> +       if (ret < 0) {
> +               release_sysdata_file(sysdata);
> +               sysdata = NULL;
> +       }
> +
> +       sysdata_file_update(sysdata);
> +
> +       *sysdata_p = sysdata;
> +
> +       return ret;
> +}
> +
> +/**
> + * sysdata_file_request - synchronous request for a system data file
> + * @name: name of the system data file
> + * @desc: system data file descriptor, it provides all the requirements
> + *     which must be met for the file being requested.
> + * @device: device for which firmware is being loaded
> + *
> + * This performs a synchronous system data file lookup with the requirements
> + * specified on @desc, if the file was found meeting the criteria requested
> + * 0 is returned. Access to the system data file data can be accessed through
> + * an optional callback set on the @desc. If the system data file is optional
> + * you must specify that on the @desc and if set you may provide an alternative
> + * callback which if set would be run if the system data file was not found.
> + *
> + * The system data file passed to the callbacks will always be NULL unless
> + * the it was found matching all the criteria on @desc. 0 is always returned
> + * if the file was found unless a callback was provided, in which case the
> + * callback's return value will be passed. Unless the desc->keep was set the
> + * kernel will release the system data file for you after your callbacks
> + * were processed.
> + *
> + * Reference counting is used during the duration of this call on both the
> + * device and module that made the request. This prevents any callers from
> + * freeing either the device or module prior to completion of this call.
> + */
> +int sysdata_file_request(const char *name,
> +                        const struct sysdata_file_desc *desc,
> +                        struct device *device)
> +{
> +       const struct sysdata_file *sysdata;
> +       const struct sysdata_file_sync_reqs *sync_reqs;
> +       int ret;
> +
> +       if (!device || !desc || !name)
> +               return -EINVAL;
> +
> +       if (desc->sync_reqs.mode != SYNCDATA_SYNC)
> +               return -EINVAL;
> +
> +       sync_reqs = &dfl_sync_reqs;
> +
> +       __module_get(sync_reqs->module);
> +       get_device(device);
> +
> +       ret = _sysdata_file_request(&sysdata, name, desc, device);
> +       if (ret && desc->optional)
> +               ret = desc_sync_opt_call_cb(desc);
> +       else
> +               ret = desc_sync_found_call_cb(desc, sysdata);
> +
> +       if (!desc->keep)
> +               release_sysdata_file(sysdata);
> +
> +       put_device(device);
> +       module_put(sync_reqs->module);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(sysdata_file_request);
> +
>  /* Async support */
>  struct firmware_work {
>         struct work_struct work;
> @@ -1360,6 +1543,114 @@ request_firmware_nowait(
>  }
>  EXPORT_SYMBOL(request_firmware_nowait);
>
> +struct sysdata_file_work {
> +       struct work_struct work;
> +       const char *name;
> +       struct sysdata_file_desc desc;
> +       struct device *device;
> +};
> +
> +static void request_sysdata_file_work_func(struct work_struct *work)
> +{
> +       struct sysdata_file_work *sys_work;
> +       const struct sysdata_file_desc *desc;
> +       const struct sysdata_file_sync_reqs *sync_reqs;
> +       const struct sysdata_file *sysdata;
> +       int ret;
> +
> +       sys_work = container_of(work, struct sysdata_file_work, work);
> +       desc = &sys_work->desc;
> +       sync_reqs = &desc->sync_reqs;
> +
> +       ret = _sysdata_file_request(&sysdata, sys_work->name,
> +                                   desc, sys_work->device);
> +       if (ret && desc->optional)
> +               desc_async_opt_call_cb(desc);
> +       else
> +               desc_async_found_call_cb(sysdata, desc);
> +
> +       if (!desc->keep)
> +               release_sysdata_file(sysdata);
> +
> +       put_device(sys_work->device);
> +       module_put(sync_reqs->module);
> +
> +       kfree_const(sys_work->name);
> +       kfree(sys_work);
> +}
> +
> +/**
> + * sysdata_file_request_async - asynchronous request for a system data file
> + * @name: name of the system data file
> + * @desc: system data file descriptor, it provides all the requirements
> + *     which must be met for the file being requested.
> + * @device: device for which firmware is being loaded
> + *
> + * This performs an asynchronous system data file lookup with the requirements
> + * specified on @desc. The request for the actual system data file lookup will
> + * be scheduled with schedule_work() to be run at a later time. 0 is returned
> + * if we were able to schedlue the work to be run.
> + *
> + * Reference counting is used during the duration of this scheduled call on
> + * both the device and module that made the request. This prevents any callers
> + * from freeing either the device or module prior to completion of the
> + * scheduled work.
> + *
> + * Access to the system data file data can be accessed through an optional
> + * callback set on the @desc. If the system data file is optional you must
> + * specify that on the @desc and if set you may provide an alternative
> + * callback which if set would be run if the system data file was not found.
> + *
> + * The system data file passed to the callbacks will always be NULL unless
> + * the it was found matching all the criteria on @desc. Unless the desc->keep
> + * was set the kernel will release the system data file for you after your
> + * callbacks were processed on the scheduled work.
> + *
> + */
> +int sysdata_file_request_async(const char *name,
> +                              const struct sysdata_file_desc *desc,
> +                              struct device *device)
> +{
> +       struct sysdata_file_work *sys_work;
> +       const struct sysdata_file_sync_reqs *sync_reqs;
> +
> +       if (!device || !desc || !name)
> +               return -EINVAL;
> +
> +       if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
> +               return -EINVAL;
> +
> +       sync_reqs = &desc->sync_reqs;
> +
> +       if (!sync_reqs->module)
> +               return -EINVAL;
> +
> +       sys_work = kzalloc(sizeof(struct sysdata_file_work), sync_reqs->gfp);
> +       if (!sys_work)
> +               return -ENOMEM;
> +
> +       sys_work->device = device;
> +       memcpy(&sys_work->desc, desc, sizeof(struct sysdata_file_desc));
> +       sys_work->name = kstrdup_const(name, sync_reqs->gfp);
> +       if (!sys_work->name) {
> +               kfree(sys_work);
> +               return -ENOMEM;
> +       }
> +
> +       if (!try_module_get(sync_reqs->module)) {
> +               kfree_const(sys_work->name);
> +               kfree(sys_work);
> +               return -EFAULT;
> +       }
> +
> +       get_device(sys_work->device);
> +       INIT_WORK(&sys_work->work, request_sysdata_file_work_func);
> +       schedule_work(&sys_work->work);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(sysdata_file_request_async);
> +
>  #ifdef CONFIG_PM_SLEEP
>  static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
>
> diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
> new file mode 100644
> index 000000000000..b4fdd941ee27
> --- /dev/null
> +++ b/include/linux/sysdata.h
> @@ -0,0 +1,212 @@
> +#ifndef _LINUX_SYSDATA_H
> +#define _LINUX_SYSDATA_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/gfp.h>
> +
> +/*
> + * System Data internals
> + *
> + * Copyright (C) 2015 Luis R. Rodriguez <mcgrof@do-not-panic.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +struct sysdata_file {
> +       size_t size;
> +       const u8 *data;
> +
> +       /* sysdata loader private fields */
> +       void *priv;
> +};
> +
> +/**
> + * enum sync_data_mode - system data mode of operation
> + *
> + * SYNCDATA_SYNC: your call to request system data is synchronous. We will
> + *     look for the system data file you have requested immediatley.
> + * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
> + *     schedule the search for your system data file to be run at a later
> + *     time.
> + */
> +enum sync_data_mode {
> +       SYNCDATA_SYNC,
> +       SYNCDATA_ASYNC,
> +};
> +
> +/* one per sync_data_mode */
> +union sysdata_file_cbs {
> +       struct {
> +               int __must_check (*found_cb)(void *, const struct sysdata_file *);
> +               void *found_context;
> +
> +               int __must_check (*opt_fail_cb)(void *);
> +               void *opt_fail_context;
> +       } sync;
> +       struct {
> +               void (*found_cb)(const struct sysdata_file *, void *);
> +               void *found_context;
> +
> +               void (*opt_fail_cb)(void *);
> +               void *opt_fail_context;
> +       } async;
> +};
> +
> +struct sysdata_file_sync_reqs {
> +       enum sync_data_mode mode;
> +       struct module *module;
> +       gfp_t gfp;
> +};
> +
> +/**
> + * struct sysdata_file_desc - system data file descriptor
> + * @optional: if true it is not a hard requirement by the caller that this
> + *     file be present. An error will not be recorded if the file is not
> + *     found.
> + * @keep: if set the caller wants to claim ownership over the system data
> + *     through one of its callbacks, it must later free it with
> + *     release_sysdata_file(). By default this is set to false and the kernel
> + *     will release the system data file for you after callback processing
> + *     has completed.
> + * @sync_reqs: synchronization requirements, this will be taken care for you
> + *     by default if you are usingy sdata_file_request(), otherwise you
> + *     should provide your own requirements.
> + *
> + * This structure is set the by the driver and passed to the system data
> + * file helpers sysdata_file_request() or sysdata_file_request_async().
> + * It is intended to carry all requirements and specifications required
> + * to complete the task to get the requested system date file to the caller.
> + * If you wish to extend functionality of system data file requests you
> + * should extend this data structure and make use of the extensions on
> + * the callers to avoid unnecessary collateral evolutions.
> + *
> + * You are allowed to provide a callback to handle if a system data file was
> + * found or not. You do not need to provide a callback. You may also set
> + * an optional flag which would enable you to declare that the system data
> + * file is optional and that if it is not found an alternative callback be
> + * run for you.
> + *
> + * Refer to sysdata_file_request() and sysdata_file_request_async() for more
> + * details.
> + */
> +struct sysdata_file_desc {
> +       bool optional;
> +       bool keep;
> +       struct sysdata_file_sync_reqs sync_reqs;
> +       const union sysdata_file_cbs cbs;
> +};
> +
> +/*
> + * We keep these template definitions to a minimum for the most
> + * popular requests.
> + */
> +
> +/* Typical sync data case */
> +#define SYSDATA_SYNC_FOUND(__found_cb, __context)                      \
> +       .cbs.sync.found_cb = __found_cb,                                \
> +       .cbs.sync.found_context = __context
> +
> +/* If you have one fallback routine */
> +#define SYSDATA_SYNC_OPT_CB(__found_cb, __context)                     \
> +       .cbs.sync.opt_fail_cb = __found_cb,                             \
> +       .cbs.sync.opt_fail_context = __context
> +
> +/*
> + * Used to define the default asynchronization requirements for
> + * sysdata_file_request_async(). Drivers can override.
> + */
> +#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context)                   \
> +       .sync_reqs = {                                                  \
> +               .mode = SYNCDATA_ASYNC,                                 \
> +               .module = THIS_MODULE,                                  \
> +               .gfp = GFP_KERNEL,                                      \
> +       },                                                              \
> +       .cbs.async = {                                                  \
> +               .found_cb = __found_cb,                                 \
> +               .found_context = __context,                             \
> +       }
> +
> +#define desc_sync_found_cb(desc)       ((desc)->cbs.sync.found_cb)
> +#define desc_sync_found_context(desc)  ((desc)->cbs.sync.found_context)
> +static inline int desc_sync_found_call_cb(const struct sysdata_file_desc *desc,
> +                                         const struct sysdata_file *sysdata)
> +{
> +       if (desc->sync_reqs.mode != SYNCDATA_SYNC)
> +               return -EINVAL;
> +       if (!desc_sync_found_cb(desc)) {
> +               if (sysdata)
> +                       return 0;
> +               return -ENOENT;
> +       }
> +       return desc_sync_found_cb(desc)(desc_sync_found_context(desc),
> +                                       sysdata);
> +}
> +
> +#define desc_sync_opt_cb(desc)         ((desc)->cbs.sync.opt_fail_cb)
> +#define desc_sync_opt_context(desc)    ((desc)->cbs.sync.opt_fail_context)
> +static inline int desc_sync_opt_call_cb(const struct sysdata_file_desc *desc)
> +{
> +       if (desc->sync_reqs.mode != SYNCDATA_SYNC)
> +               return -EINVAL;
> +       if (!desc_sync_opt_cb(desc))
> +               return 0;
> +       return desc_sync_opt_cb(desc)(desc_sync_opt_context(desc));
> +}
> +
> +#define desc_async_found_cb(desc)      ((desc)->cbs.async.found_cb)
> +#define desc_async_found_context(desc) ((desc)->cbs.async.found_context)
> +static inline void desc_async_found_call_cb(const struct sysdata_file *sysdata,
> +                                           const struct sysdata_file_desc *desc)
> +{
> +       if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
> +               return;
> +       if (!desc_async_found_cb(desc))
> +               return;
> +       desc_async_found_cb(desc)(sysdata, desc_async_found_context(desc));
> +}
> +
> +#define desc_async_opt_cb(desc)                ((desc)->cbs.async.opt_fail_cb)
> +#define desc_async_opt_context(desc)   ((desc)->cbs.async.opt_fail_context)
> +static inline void desc_async_opt_call_cb(const struct sysdata_file_desc *desc)
> +{
> +       if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
> +               return;
> +       if (!desc_async_opt_cb(desc))
> +               return;
> +       desc_async_opt_cb(desc)(desc_async_opt_context(desc));
> +}
> +
> +
> +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
> +int sysdata_file_request(const char *name,
> +                        const struct sysdata_file_desc *desc,
> +                        struct device *device);
> +int sysdata_file_request_async(const char *name,
> +                              const struct sysdata_file_desc *desc,
> +                              struct device *device);
> +void release_sysdata_file(const struct sysdata_file *sysdata);
> +#else
> +static inline int sysdata_file_request(const char *name,
> +                                      const struct sysdata_file_desc *desc,
> +                                      struct device *device)
> +{
> +       return -EINVAL;
> +}
> +
> +static inline int sysdata_file_request_async(const char *name,
> +                                            const struct sysdata_file_desc *desc,
> +                                            struct device *device);
> +{
> +       return -EINVAL;
> +}
> +
> +static inline void release_sysdata_file(const struct sysdata_file *sysdata)
> +{
> +}
> +#endif
> +
> +#endif /* _LINUX_SYSDATA_H */
> --
> 2.6.2
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v3 1/5] firmware: generalize "firmware" as "system data" helpers

[Kees Cook]

On Wed, Dec 23, 2015 at 1:34 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Historically firmware_class code was added to help
> get device driver firmware binaries but these days
> request_firmware*() helpers are being repurposed for
> general system data needed by the kernel.
>
> Annotate this before we extend firmare_class more,
> as this is expected. We want to generalize the code
> as much as possible.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Vojtěch Pavlík <vojtech@suse.cz>
> Cc: Kyle McMartin <kyle@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  drivers/base/firmware_class.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450e75bd..6f5fcda71a60 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device *device,
>                 rc = fw_read_file_contents(file, buf);
>                 fput(file);
>                 if (rc)
> -                       dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> -                               path, rc);
> +                       dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
> +                                path, rc);

Since dev_warn should already be prefixing the string, I would think
"firmware, " should just be removed entirely instead of replaced?

-Kees

>                 else
>                         break;
>         }
>         __putname(path);
>
>         if (!rc) {
> -               dev_dbg(device, "firmware: direct-loading firmware %s\n",
> +               dev_dbg(device, "system data: direct-loading firmware %s\n",
>                         buf->fw_id);
>                 mutex_lock(&fw_lock);
>                 set_bit(FW_STATUS_DONE, &buf->status);
> @@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>         }
>
>         if (fw_get_builtin_firmware(firmware, name)) {
> -               dev_dbg(device, "firmware: using built-in firmware %s\n", name);
> +               dev_dbg(device, "system data: using built-in system data%s\n", name);
>                 return 0; /* assigned */
>         }
>
> --
> 2.6.2
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v3 2/5] firmware: move completing fw into a helper

[Josh Boyer]

On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> This will be re-used later through a new extensible interface.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>

josh

> ---
>  drivers/base/firmware_class.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 6f5fcda71a60..d8148aa89b01 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -322,6 +322,15 @@ fail:
>         return rc;
>  }
>
> +static void fw_finish_direct_load(struct device *device,
> +                                 struct firmware_buf *buf)
> +{
> +       mutex_lock(&fw_lock);
> +       set_bit(FW_STATUS_DONE, &buf->status);
> +       complete_all(&buf->completion);
> +       mutex_unlock(&fw_lock);
> +}
> +
>  static int fw_get_filesystem_firmware(struct device *device,
>                                        struct firmware_buf *buf)
>  {
> @@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device *device,
>         if (!rc) {
>                 dev_dbg(device, "system data: direct-loading firmware %s\n",
>                         buf->fw_id);
> -               mutex_lock(&fw_lock);
> -               set_bit(FW_STATUS_DONE, &buf->status);
> -               complete_all(&buf->completion);
> -               mutex_unlock(&fw_lock);
> +               fw_finish_direct_load(device, buf);
>         }
>
>         return rc;
> --
> 2.6.2
>

Re: [PATCH v3 3/5] firmware: fold successful fw read early

[Josh Boyer]

On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: David Howells <dhowells@redhat.com>
>
> We'll be folding in some more checks on fw_read_file_contents(),
> this will make the success case easier to follow.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>

> ---
>  drivers/base/firmware_class.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d8148aa89b01..e10a5349aa61 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device *device,
>                         continue;
>                 rc = fw_read_file_contents(file, buf);
>                 fput(file);
> -               if (rc)
> +               if (rc == 0) {
> +                       dev_dbg(device, "system data: direct-loading firmware %s\n",
> +                               buf->fw_id);
> +                       fw_finish_direct_load(device, buf);
> +                       goto out;
> +               } else
>                         dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
>                                  path, rc);
> -               else
> -                       break;
>         }
> +out:
>         __putname(path);
>
> -       if (!rc) {
> -               dev_dbg(device, "system data: direct-loading firmware %s\n",
> -                       buf->fw_id);
> -               fw_finish_direct_load(device, buf);
> -       }
> -
>         return rc;
>  }
>
> --
> 2.6.2
>

Re: [PATCH v3 5/5] firmware: add an extensible system data helpers

[Luis R. Rodriguez]

On Thu, Dec 24, 2015 at 06:26:39AM +0800, kbuild test robot wrote:
> reproduce: make htmldocs
> >> drivers/base/firmware_class.c:1336: warning: No description found for parameter 'sysdata'
>   1331	/**
>   1332	 * release_sysdata_file: - release the resource associated with the sysdata file
>   1333	 * @sysdata_file: sysdata resource to release
>   1334	 **/
>   1335	void release_sysdata_file(const struct sysdata_file *sysdata)
> > 1336	{

This was a kdoc warning, I've fixed it and this will be part of my
v5 respin.

  Luis

Re: [PATCH v3 4/5] firmware: generalize reading file contents as a helper

[Luis R. Rodriguez]

Greg,

Due to Mimi's awesome work on generalizing a common VFS file read [0] as
described as possible in the commit log below this patch will be dropped in
preference for Mimi's VFS work to go in first.

Mimi,

since your generic VFS routine is pretty much identical to fw_read_file()
defined here you could make use of hunks #2 and #3 below. To make this more
legible it does depend on another patch which simplifies things. You can feel
free to pick that up as part of your series if you see it helps. If it doesn't
help that's fine too, I'll just submit later, but since you're digging in the
same way as I was I figured this could help.

I'll copy on you the other patch next.

[0] http//lkml.kernel.org/r/1453129886-20192-1-git-send-email-zohar@linux.vnet.ibm.com

  Luis

On Wed, Dec 23, 2015 at 01:34:56PM -0800, Luis R. Rodriguez wrote:
> From: David Howells <dhowells@redhat.com>
> 
> We'll want to reuse this same code later in order to read
> two separate types of file contents. This generalizes
> fw_read_file_contents() for reading a file and rebrands it
> as fw_read_file(). This new caller is now generic: the path
> used can be arbitrary and the caller is also agnostic to the
> firmware_class code now, this begs the possibility of code
> re-use with other similar callers in the kernel. For instance
> in the future we may want to share a solution with:
> 
>     - firmware_class: fw_read_file()
>     - module: kernel_read()
>     - kexec: copy_file_fd()
> 
> While at it this also cleans up the exit paths on fw_read_file().
> 
> Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  drivers/base/firmware_class.c | 62 +++++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index e10a5349aa61..cd51a90ed012 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -291,34 +291,51 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>  
> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +/*
> + * Read the contents of a file.
> + */
> +static int fw_read_file(const char *path, void **_buf, size_t *_size)
>  {
> -	int size;
> +	struct file *file;
> +	size_t size;
>  	char *buf;
>  	int rc;
>  
> +	file = filp_open(path, O_RDONLY, 0);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	rc = -EINVAL;
>  	if (!S_ISREG(file_inode(file)->i_mode))
> -		return -EINVAL;
> +		goto err_file;
>  	size = i_size_read(file_inode(file));
>  	if (size <= 0)
> -		return -EINVAL;
> +		goto err_file;
> +	rc = -ENOMEM;
>  	buf = vmalloc(size);
>  	if (!buf)
> -		return -ENOMEM;
> +		goto err_file;
> +
>  	rc = kernel_read(file, 0, buf, size);
> +	if (rc < 0)
> +		goto err_buf;
>  	if (rc != size) {
> -		if (rc > 0)
> -			rc = -EIO;
> -		goto fail;
> +		rc = -EIO;
> +		goto err_buf;
>  	}
> +
>  	rc = security_kernel_fw_from_file(file, buf, size);
>  	if (rc)
> -		goto fail;
> -	fw_buf->data = buf;
> -	fw_buf->size = size;
> +		goto err_buf;
> +
> +	*_buf = buf;
> +	*_size = size;
>  	return 0;
> -fail:
> +
> +err_buf:
>  	vfree(buf);
> +err_file:
> +	fput(file);
>  	return rc;
>  }
>  
> @@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
>  }
>  
>  static int fw_get_filesystem_firmware(struct device *device,
> -				       struct firmware_buf *buf)
> +				      struct firmware_buf *buf)
>  {
>  	int i, len;
>  	int rc = -ENOENT;
> -	char *path;
> +	char *path = NULL;
>  
>  	path = __getname();
>  	if (!path)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Try each possible firmware blob in turn till one doesn't produce
> +	 * ENOENT.
> +	 */
>  	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> -		struct file *file;
> -
>  		/* skip the unset customized path */
>  		if (!fw_path[i][0])
>  			continue;
> @@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device *device,
>  			break;
>  		}
>  
> -		file = filp_open(path, O_RDONLY, 0);
> -		if (IS_ERR(file))
> -			continue;
> -		rc = fw_read_file_contents(file, buf);
> -		fput(file);
> +		rc = fw_read_file(path, &buf->data, &buf->size);
>  		if (rc == 0) {
>  			dev_dbg(device, "system data: direct-loading firmware %s\n",
>  				buf->fw_id);
>  			fw_finish_direct_load(device, buf);
>  			goto out;
> -		} else
> +		} else if (rc != -ENOENT) {
>  			dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
>  				 path, rc);
> +			goto out;
> +		}
>  	}
>  out:
>  	__putname(path);
> -
>  	return rc;
>  }
>  
> -- 
> 2.6.2
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [PATCH v3 3/5] firmware: fold successful fw read early

[Luis R. Rodriguez]

On Mon, Jan 04, 2016 at 03:48:29PM -0500, Josh Boyer wrote:
> On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: David Howells <dhowells@redhat.com>
> >
> > We'll be folding in some more checks on fw_read_file_contents(),
> > this will make the success case easier to follow.
> >
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> 
> Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>

Thanks Josh.

Mimi, this is the other patch that  I was referring to.

  Luis
> 
> > ---
> >  drivers/base/firmware_class.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index d8148aa89b01..e10a5349aa61 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device *device,
> >                         continue;
> >                 rc = fw_read_file_contents(file, buf);
> >                 fput(file);
> > -               if (rc)
> > +               if (rc == 0) {
> > +                       dev_dbg(device, "system data: direct-loading firmware %s\n",
> > +                               buf->fw_id);
> > +                       fw_finish_direct_load(device, buf);
> > +                       goto out;
> > +               } else
> >                         dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
> >                                  path, rc);
> > -               else
> > -                       break;
> >         }
> > +out:
> >         __putname(path);
> >
> > -       if (!rc) {
> > -               dev_dbg(device, "system data: direct-loading firmware %s\n",
> > -                       buf->fw_id);
> > -               fw_finish_direct_load(device, buf);
> > -       }
> > -
> >         return rc;
> >  }
> >
> > --
> > 2.6.2
> >
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [PATCH v3 5/5] firmware: add an extensible system data helpers

[Luis R. Rodriguez]

On Mon, Jan 04, 2016 at 12:31:58PM -0800, Kees Cook wrote:
> On Wed, Dec 23, 2015 at 1:34 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > In order to try to help phase out user mode helpers this makes no use of
> > the old user mode helper code *at all*, and if we wish to can easily
> > phase this code out with time then.
> 
> So these are basically wrappers around the existing firmware loading routines?

No, Greg has noted we cannot get rid of the usermode helper [0]. In fact at
kernel summit he mentioned there are a series of upcoming valid users who seem
to *want* it.  Even Linus has called for deprecating the usermode helper [1]
entirely if possible. This work tries to enable such prospects despite some
needing the usermode helper by enabling callers that *need* the usermode helper
to use the crappy usermode helper and letting us slowly dig that into a dark
corner. This paves the path with a shiny extensible API with prospects of
future features (fw signingin will be one) without use of the usermode helper
at all, the extensible API enables new extensions by avoiding unnecessary
collateral evolutions as this code / features get added. This provides a clean
an way to enable folks who do wish to deprecate and the usermode helper to do
so and provides carrots for doing that.
 
[0] https://marc.info/?i=20151006090821.GB9030%40kroah.com
[1] https://marc.info/?l=linux-kernel&m=144095832412928

  Luis

Re: [PATCH v3 3/5] firmware: fold successful fw read early

[Mimi Zohar]

On Fri, 2016-01-22 at 02:45 +0100, Luis R. Rodriguez wrote:
> On Mon, Jan 04, 2016 at 03:48:29PM -0500, Josh Boyer wrote:
> > On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
> > <mcgrof@do-not-panic.com> wrote:
> > > From: David Howells <dhowells@redhat.com>
> > >
> > > We'll be folding in some more checks on fw_read_file_contents(),
> > > this will make the success case easier to follow.
> > >
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > 
> > Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>
> 
> Thanks Josh.
> 
> Mimi, this is the other patch that  I was referring to.

After this patch set, replacing the fw_read_file calls with the
kernel_read_file_from_path() version is straight forward.  Nice!

Mimi

Re: [PATCH v3 3/5] firmware: fold successful fw read early

[Luis R. Rodriguez]

On Fri, Jan 22, 2016 at 3:56 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Fri, 2016-01-22 at 02:45 +0100, Luis R. Rodriguez wrote:
>> On Mon, Jan 04, 2016 at 03:48:29PM -0500, Josh Boyer wrote:
>> > On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
>> > <mcgrof@do-not-panic.com> wrote:
>> > > From: David Howells <dhowells@redhat.com>
>> > >
>> > > We'll be folding in some more checks on fw_read_file_contents(),
>> > > this will make the success case easier to follow.
>> > >
>> > > Signed-off-by: David Howells <dhowells@redhat.com>
>> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
>> >
>> > Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>
>>
>> Thanks Josh.
>>
>> Mimi, this is the other patch that  I was referring to.
>
> After this patch set, replacing the fw_read_file calls with the
> kernel_read_file_from_path() version is straight forward.  Nice!

Great, I take it you will pick it up then ? :)

  Luis

Re: [PATCH v3 1/5] firmware: generalize "firmware" as "system data" helpers

[Luis R. Rodriguez]

On Mon, Jan 04, 2016 at 12:41:07PM -0800, Kees Cook wrote:
> On Wed, Dec 23, 2015 at 1:34 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > Historically firmware_class code was added to help
> > get device driver firmware binaries but these days
> > request_firmware*() helpers are being repurposed for
> > general system data needed by the kernel.
> >
> > Annotate this before we extend firmare_class more,
> > as this is expected. We want to generalize the code
> > as much as possible.
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Casey Schaufler <casey@schaufler-ca.com>
> > Cc: Ming Lei <ming.lei@canonical.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Vojtěch Pavlík <vojtech@suse.cz>
> > Cc: Kyle McMartin <kyle@kernel.org>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  drivers/base/firmware_class.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8524450e75bd..6f5fcda71a60 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device *device,
> >                 rc = fw_read_file_contents(file, buf);
> >                 fput(file);
> >                 if (rc)
> > -                       dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> > -                               path, rc);
> > +                       dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
> > +                                path, rc);
> 
> Since dev_warn should already be prefixing the string, I would think
> "firmware, " should just be removed entirely instead of replaced?

Its a good point, the messages could also be simplified further. Here's
a more generic form and patch description that I'll wrap into a v4
iteration after Mimi's patches go in.

[PATCH] firmware: simplify generic "firmware" helpers

Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.

To make the generic set of routines that will be shared
with generic system data helpers simplify a few of
the shared debug and warn print outs, the prints already
have the device associated as dev_*() helpers are used.
This will help generalize shared code as much as possible.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vojtěch Pavlík <vojtech@suse.cz>
Cc: Kyle McMartin <kyle@kernel.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/firmware_class.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450e75bd..3358f5df926f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device *device,
 		rc = fw_read_file_contents(file, buf);
 		fput(file);
 		if (rc)
-			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
-				path, rc);
+			dev_warn(device, "loading %s failed with error %d\n",
+				 path, rc);
 		else
 			break;
 	}
 	__putname(path);
 
 	if (!rc) {
-		dev_dbg(device, "firmware: direct-loading firmware %s\n",
+		dev_dbg(device, "direct-loading %s\n",
 			buf->fw_id);
 		mutex_lock(&fw_lock);
 		set_bit(FW_STATUS_DONE, &buf->status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
-		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+		dev_dbg(device, "using built-in %s\n", name);
 		return 0; /* assigned */
 	}
 
-- 
2.7.0