firmware: add driver data API

As the firmware API evolves we keep extending functions with more arguments. Stop this nonsense by proving an extensible data structure which can be used to represent both user parameters and private internal parameters.

We introduce 3 data structures:

o struct driver_data_req_params - used for user specified parameters
o struct driver_data_priv_params - used for internal use
o struct driver_data_params - stiches both of the the above together, also only for internal use

This starts off by just making the existing APIs use the new data structures, it will make subsequent changes easier to review which will be adding new flexible APIs.

This commit should introduces no functional changes (TM).

Signed-off-by Luis R. Rodriguez <[email protected]>

---

drivers/base/firmware_class.c +0 -0

index 54fc4c42f126..f702566554e1 100644

drivers/base/firmware_class.c +20 -0

@@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+struct driver_data_priv_params {
+	bool use_fallback;
+	bool use_fallback_uevent;
+	bool no_cache;
+	void *alloc_buf;
+	size_t alloc_buf_size;
+};
+
+struct driver_data_params {
+	const struct firmware *driver_data;
+	const struct driver_data_req_params req_params;
+	struct driver_data_priv_params priv_params;
+};
+
+/*
+ * These are kept to remain backward compatible with old behaviour. Do not
+ * modify them unless you know what you are doing. These are to be used only
+ * by the old API, so:
+ *
+ * Old sync APIs:

drivers/base/firmware_class.c +2 -0

@@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 			  struct device *device, void *buf, size_t size)
 {
 	int ret;
+	struct driver_data_params data_params = {
+		__DATA_REQ_FIRMWARE_BUF(buf, size),

drivers/base/firmware_class.c +0 -0

 	return ret;
 }
 EXPORT_SYMBOL(request_firmware_into_buf);

include/linux/driver_data.h new file +14 -0

new file mode 100644
index 000000000000..c3d3a4129838
@@ -0,0 +1,88 @@
+#ifndef _LINUX_DRIVER_DATA_H
+#define _LINUX_DRIVER_DATA_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/gfp.h>
+#include <linux/device.h>
+
+/*
+ * Driver Data internals
+ *
+ * Copyright (C) 2017 Luis R. Rodriguez <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it

Quoted 2 lines · Luis R. Rodriguez

Signed-off-by Luis R. Rodriguez <[email protected]>

---

Looks fine with a few nitpicks.

Quoted 4 lines

drivers/base/firmware_class.c +0 -0

index 54fc4c42f126..f702566554e1 100644

[...]

Quoted 28 lines

drivers/base/firmware_class.c +24 -0

@@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+struct driver_data_priv_params {
+	bool use_fallback;
+	bool use_fallback_uevent;
+	bool no_cache;
+	void *alloc_buf;
+	size_t alloc_buf_size;
+};
+
+struct driver_data_params {
+	const struct firmware *driver_data;
+	const struct driver_data_req_params req_params;
+	struct driver_data_priv_params priv_params;
+};
+
+/*
+ * These are kept to remain backward compatible with old behaviour. Do not
+ * modify them unless you know what you are doing. These are to be used only
+ * by the old API, so:
+ *
+ * Old sync APIs:
+ * 	o request_firmware():		__DATA_REQ_FIRMWARE()
+ * 	o request_firmware_direct():	__DATA_REQ_FIRMWARE_DIRECT()
+ *	o request_firmware_into_buf():	__DATA_REQ_FIRMWARE_BUF()
+ *

drivers/base/firmware_class.c MODULE_AUTHOR("Manuel Estrada Sainz"); · line 73

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+ * Old async API:
+ *	o request_firmware_nowait():	__DATA_REQ_FIRMWARE_NOWAIT()
+ */
+#define __DATA_REQ_FIRMWARE()						\
+	.priv_params = {						\
+		.use_fallback = !!FW_OPT_FALLBACK,			\

use_fallback is a boolean, so you don't need the double negation here.

[...]

Quoted 8 lines

drivers/base/firmware_class.c +3 -0

@@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 			  struct device *device, void *buf, size_t size)
 {
 	int ret;
+	struct driver_data_params data_params = {
+		__DATA_REQ_FIRMWARE_BUF(buf, size),
+	};
 

drivers/base/firmware_class.c request_firmware_into_buf(const struct firmware **firmware_p, const char *name, · line 1444

Quoted 8 lines

drivers/base/firmware_class.c +3 -3

 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK |
-				FW_OPT_NOCACHE);
+	ret = _request_firmware(firmware_p, name, &data_params, device);
 	module_put(THIS_MODULE);
+
+

Double empty-lines here.

Quoted 4 lines

drivers/base/firmware_class.c +0 -0

 	return ret;
 }
 EXPORT_SYMBOL(request_firmware_into_buf);

[...]

Quoted 24 lines

include/linux/driver_data.h new file +18 -0

new file mode 100644
index 000000000000..c3d3a4129838
@@ -0,0 +1,88 @@
+#ifndef _LINUX_DRIVER_DATA_H
+#define _LINUX_DRIVER_DATA_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/gfp.h>
+#include <linux/device.h>
+
+/*
+ * Driver Data internals
+ *
+ * Copyright (C) 2017 Luis R. Rodriguez <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of copyleft-next (version 0.3.1 or later) as published
+ * at http://copyleft-next.org/.
+ */
+

include/linux/driver_data.h line 24

Quoted 6 lines

include/linux/driver_data.h new file +6 -0

+/**
+ * enum driver_data_mode - driver data mode of operation
+ *
+ * DRIVER_DATA_SYNC: your call to request driver data is synchronous. We will
+ *	look for the driver data file you have requested immediatley.
+ * DRIVER_DATA_ASYNC: your call to request driver data is asynchronous. We will

It should be @DRIVER_DATA_SYNC and @DRIVER_DATA_ASYNC here.

The firmware API does not scale well: when new features are added we either add a new exported symbol or extend the arguments of existing routines. For the later case this 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". There are other subsystems which would like to make use of the firmware APIs for similar things and its clearly not firmware, but have different requirements and criteria which they'd like to be met for the requested file.

An extensible API is in order:

The driver data API accepts that there are only two types of requests:

a) synchronous requests
b) asynchronous requests

Both requests may have a different requirements which must be met. These requirements can be described in the struct driver_data_req_params. This struct is expected to be extended over time to support different requirements as the kernel evolves.

After a bit of hard work the new interface has been wrapped onto the functionality. The fallback mechanism has been kept out of the new API currently because it requires just a bit more grooming and documentation given new considerations and requirements. Adding support for it will be rather easy now that the new API sits ontop of the old one. The request_firmware_into_buf() API also is not enabled on the new API but it is rather easy to do so -- this call has no current existing users upstream though. Support will be provided once we add a respective series of test cases against it and find a proper upstream user for it.

The flexible API also adds a few new bells and whistles:

- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the requirements params. The new driver data API is able to free the driver data file for you by requiring a consumer callback for the driver data file.
- Allows both asynchronous and synchronous request to specify that driver data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- the driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- A firmware API framework is provided to enable daisy chaining a series of requests for firmware on a range of supported APIs.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 357 +++++++++++++++++++++
 include/linux/driver_data.h                        | 202 +++++++++++-
 include/linux/firmware.h                           |   2 +
 7 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst

Documentation/driver-api/firmware/driver_data.rst new file +0 -0

new file mode 100644
index 000000000000..08407b7568fe

Documentation/driver-api/firmware/driver_data.rst new file +7 -0

+
+Reference counting and releasing the driver data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the driver data requests. This prevents removal
+of the device and module making the driver data request call until the

drivers/base/firmware_class.c +0 -0

index f702566554e1..cc3c2247980c 100644

drivers/base/firmware_class.c +6 -0

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static int _driver_data_request_api(struct driver_data_params *params,
+				    struct device *device,
+				    const char *name)
+{
+	struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;

drivers/base/firmware_class.c +40 -0

+	for (priv_params->api = api_max;
+	     priv_params->api >= req_params->api_min;
+	     priv_params->api--) {
+		if (req_params->api_name_postfix)
+			try_name = kasprintf(GFP_KERNEL, "%s%d%s",
+					     name,
+					     priv_params->api,
+					     req_params->api_name_postfix);
+		else
+			try_name = kasprintf(GFP_KERNEL, "%s%d",
+					     name,
+					     priv_params->api);
+		if (!try_name)
+			return -ENOMEM;
+		ret = _request_firmware(&params->driver_data, try_name,
+					params, device);
+		kfree(try_name);
+
+		if (!ret)
+			break;
+
+		release_firmware(params->driver_data);
+
+		/*
+		 * Only chug on with the API revision hunt if the file we
+		 * looked for really was not present. In case of memory issues
+		 * or other related system issues we want to bail right away
+		 * to not put strain on the system.
+		 */
+		if (ret != -ENOENT)
+			break;
+
+		if (!priv_params->api)
+			break;
+	}
+
+	return ret;
+}
+
+/**

drivers/base/firmware_class.c +19 -0

 If the driver data is optional
+ * you must specify that on @params and if set you may provide an alternative
+ * callback which if set would be run if the driver data was not found.
+ *
+ * The driver data passed to the callbacks will be NULL unless it was
+ * found matching all the criteria on @params. 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 params->keep was set the kernel will
+ * release the driver data 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 driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;

drivers/base/firmware_class.c +34 -0

+/**
+ * driver_data_request_async - asynchronous request for a driver data file
+ * @name: name of the driver data file
+ * @req_params: driver data file request parameters, 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 driver data file lookup with the requirements
+ * specified on @req_params. The request for the actual driver data file lookup
+ * will be scheduled with schedule_work() to be run at a later time. 0 is
+ * returned if we were able to asynchronously schedlue your 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 driver data file data can be accessed through an optional
+ * callback set on the @req_params. If the driver data file is optional you
+ * must specify that on @req_params and if set you may provide an alternative
+ * callback which if set would be run if the driver data file was not found.
+ *
+ * The driver data file passed to the callbacks will always be NULL unless it
+ * was found matching all the criteria on @req_params. Unless the desc->keep
+ * was set the kernel will release the driver data file for you after your
+ * callbacks were processed on the scheduled work.
+ */
+int driver_data_request_async(const char *name,
+			      const struct driver_data_req_params *req_params,
+			      struct device *device)
+{
+	struct firmware_work *driver_work;
+	const struct driver_data_reqs *sync_reqs;
+	struct firmware_work driver_work_stack = {

Quoted 60 lines · Luis R. Rodriguez

The firmware API does not scale well: when new features are added we either add a new exported symbol or extend the arguments of existing routines. For the later case this 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". There are other subsystems which would like to make use of the firmware APIs for similar things and its clearly not firmware, but have different requirements and criteria which they'd like to be met for the requested file.

An extensible API is in order:

The driver data API accepts that there are only two types of requests:

a) synchronous requests
b) asynchronous requests

Both requests may have a different requirements which must be met. These requirements can be described in the struct driver_data_req_params. This struct is expected to be extended over time to support different requirements as the kernel evolves.

After a bit of hard work the new interface has been wrapped onto the functionality. The fallback mechanism has been kept out of the new API currently because it requires just a bit more grooming and documentation given new considerations and requirements. Adding support for it will be rather easy now that the new API sits ontop of the old one. The request_firmware_into_buf() API also is not enabled on the new API but it is rather easy to do so -- this call has no current existing users upstream though. Support will be provided once we add a respective series of test cases against it and find a proper upstream user for it.

The flexible API also adds a few new bells and whistles:

- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the requirements params. The new driver data API is able to free the driver data file for you by requiring a consumer callback for the driver data file.
- Allows both asynchronous and synchronous request to specify that driver data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- the driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- A firmware API framework is provided to enable daisy chaining a series of requests for firmware on a range of supported APIs.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 357 +++++++++++++++++++++
 include/linux/driver_data.h                        | 202 +++++++++++-
 include/linux/firmware.h                           |   2 +
 7 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst

Documentation/driver-api/firmware/driver_data.rst line 5

Quoted 11 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

new file mode 100644
index 000000000000..08407b7568fe
@@ -0,0 +1,77 @@
+===============
+driver_data API
+===============
+
+Users of firmware request APIs has grown to include users which are not

Grammar. Maybe "The usage of firmware..."

Documentation/driver-api/firmware/driver_data.rst line 10

Quoted 6 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+looking for "firmware", but instead general driver data files which have
+been kept oustide of the kernel. The driver data APIs addresses rebranding
+of firmware as generic driver data files, and provides a flexible API which
+mitigates collateral evolutions on the kernel as new functionality is
+introduced.

This looks more like a commit message than an introduction to the feature. In the future, we won't care why this was introduced, but we want to know what it is and how it can be used.

Documentation/driver-api/firmware/driver_data.rst line 15

Quoted 5 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for driver data requests:

"only" seems irrelevant here.

Documentation/driver-api/firmware/driver_data.rst line 28

Quoted 13 lines

Documentation/driver-api/firmware/driver_data.rst new file +13 -0

+
+  * synchronous  - driver_data_request()
+  * asynchronous - driver_data_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure is expected to grow as
+new requirements grow.

Again, not sure it's relevant to know that it can grow. For documentation purposes, the important is the *now*.

Quoted 8 lines

Documentation/driver-api/firmware/driver_data.rst new file +7 -0

+
+Reference counting and releasing the driver data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the driver data requests. This prevents removal
+of the device and module making the driver data request call until the

Documentation/driver-api/firmware/driver_data.rst line 45

Quoted 10 lines

Documentation/driver-api/firmware/driver_data.rst new file +10 -0

+driver 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 driver data request API currently mimics this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs. In the future we may enable the ability to also refcount the
+caller's module as well. Likewise in the future we may enable asynchronous
+calls to refcount the firmware_class module.

Ditto. Maybe you could move all the "future" references to the "Tracking development enhancements and ideas" section?

[...]

Quoted 4 lines

drivers/base/firmware_class.c +0 -0

index f702566554e1..cc3c2247980c 100644

[...]

Quoted 10 lines

drivers/base/firmware_class.c +6 -0

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static int _driver_data_request_api(struct driver_data_params *params,
+				    struct device *device,
+				    const char *name)
+{
+	struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1489

Quoted 10 lines

drivers/base/firmware_class.c +10 -0

+	int ret;
+	char *try_name;
+	u8 api_max;
+
+	if (priv_params->retry_api) {
+		if (!priv_params->api)
+			return -ENOENT;
+		api_max = priv_params->api - 1;
+	} else
+		api_max = req_params->api_max;

Braces.

Quoted 33 lines

drivers/base/firmware_class.c +33 -0

+	for (priv_params->api = api_max;
+	     priv_params->api >= req_params->api_min;
+	     priv_params->api--) {
+		if (req_params->api_name_postfix)
+			try_name = kasprintf(GFP_KERNEL, "%s%d%s",
+					     name,
+					     priv_params->api,
+					     req_params->api_name_postfix);
+		else
+			try_name = kasprintf(GFP_KERNEL, "%s%d",
+					     name,
+					     priv_params->api);
+		if (!try_name)
+			return -ENOMEM;
+		ret = _request_firmware(&params->driver_data, try_name,
+					params, device);
+		kfree(try_name);
+
+		if (!ret)
+			break;
+
+		release_firmware(params->driver_data);
+
+		/*
+		 * Only chug on with the API revision hunt if the file we
+		 * looked for really was not present. In case of memory issues
+		 * or other related system issues we want to bail right away
+		 * to not put strain on the system.
+		 */
+		if (ret != -ENOENT)
+			break;
+
+		if (!priv_params->api)

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1531

Quoted 8 lines

drivers/base/firmware_class.c +8 -0

+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * driver_data_request - synchronous request for a driver data file

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1533

Quoted 2 lines

drivers/base/firmware_class.c +2 -0

+ * @name: name of the driver data file
+ * @params: driver data parameters, it provides all the requirements

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw)

Quoted 7 lines

drivers/base/firmware_class.c +7 -0

+ *	parameters which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ *
+ * This performs a synchronous driver data lookup with the requirements
+ * specified on @params, if the file was found meeting the criteria requested
+ * 0 is returned. Access to the driver data data can be accessed through
+ * an optional callback set on the @desc.

Huh? This last sentence seems wrong, I don't even see a @desc anywhere.

Quoted 20 lines

drivers/base/firmware_class.c +19 -0

 If the driver data is optional
+ * you must specify that on @params and if set you may provide an alternative
+ * callback which if set would be run if the driver data was not found.
+ *
+ * The driver data passed to the callbacks will be NULL unless it was
+ * found matching all the criteria on @params. 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 params->keep was set the kernel will
+ * release the driver data 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 driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1569

Quoted 10 lines

drivers/base/firmware_class.c +10 -0

+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;

Why do you need to check this here? If the caller is calling _sync(), it's because that's what it needs. This mode value here seems redundant.

OTOH, if you do have a reason for this value, then you could use driver_data_request_sync() in this if.

Quoted 35 lines

drivers/base/firmware_class.c +34 -0

+/**
+ * driver_data_request_async - asynchronous request for a driver data file
+ * @name: name of the driver data file
+ * @req_params: driver data file request parameters, 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 driver data file lookup with the requirements
+ * specified on @req_params. The request for the actual driver data file lookup
+ * will be scheduled with schedule_work() to be run at a later time. 0 is
+ * returned if we were able to asynchronously schedlue your 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 driver data file data can be accessed through an optional
+ * callback set on the @req_params. If the driver data file is optional you
+ * must specify that on @req_params and if set you may provide an alternative
+ * callback which if set would be run if the driver data file was not found.
+ *
+ * The driver data file passed to the callbacks will always be NULL unless it
+ * was found matching all the criteria on @req_params. Unless the desc->keep
+ * was set the kernel will release the driver data file for you after your
+ * callbacks were processed on the scheduled work.
+ */
+int driver_data_request_async(const char *name,
+			      const struct driver_data_req_params *req_params,
+			      struct device *device)
+{
+	struct firmware_work *driver_work;
+	const struct driver_data_reqs *sync_reqs;
+	struct firmware_work driver_work_stack = {

drivers/base/firmware_class.c request_firmware_nowait( · line 1876

Quoted 10 lines

drivers/base/firmware_class.c +10 -0

+		.data_params.req_params = *req_params,
+		//.device = device,
+		//.name = name,
+	};
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_ASYNC)
+		return -EINVAL;

Same here.

--
Luca.

Quoted 10 lines · Luciano

Documentation/driver-api/firmware/driver_data.rst line 28

Quoted 6 lines · Luis R. Rodriguez

Documentation/driver-api/firmware/driver_data.rst new file +6 -0

+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure is expected to grow as
+new requirements grow.

Again, not sure it's relevant to know that it can grow. For documentation purposes, the important is the *now*.

There seem to be a couple of new features/parameters. Why not list them now:
* optional
* keep
* API versioning

I will add 'data(firmware) signing' here afterward.

Quoted 2 lines

Quoted 2 lines

+/**
+ * driver_data_request - synchronous request for a driver data file

driver_data_request_sync

Quoted 2 lines

Quoted 2 lines

+ * @name: name of the driver data file
+ * @params: driver data parameters, it provides all the requirements

req_params

Quoted 21 lines

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+int driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1569

Quoted 10 lines

drivers/base/firmware_class.c +10 -0

+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;

Why do you need to check this here? If the caller is calling _sync(), it's because that's what it needs. This mode value here seems redundant.

Quoted 2 lines

OTOH, if you do have a reason for this value, then you could use driver_data_request_sync() in this if.

I think two functions, driver_data_request_[a]sync(), can be unified into one:
int driver_data_request(const char *name,
const struct driver_data_req_params *req_params, struct device *device)

Quoted 60 lines · Luis R. Rodriguez

The firmware API does not scale well: when new features are added we either add a new exported symbol or extend the arguments of existing routines. For the later case this 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". There are other subsystems which would like to make use of the firmware APIs for similar things and its clearly not firmware, but have different requirements and criteria which they'd like to be met for the requested file.

An extensible API is in order:

The driver data API accepts that there are only two types of requests:

a) synchronous requests
b) asynchronous requests

Both requests may have a different requirements which must be met. These requirements can be described in the struct driver_data_req_params. This struct is expected to be extended over time to support different requirements as the kernel evolves.

After a bit of hard work the new interface has been wrapped onto the functionality. The fallback mechanism has been kept out of the new API currently because it requires just a bit more grooming and documentation given new considerations and requirements. Adding support for it will be rather easy now that the new API sits ontop of the old one. The request_firmware_into_buf() API also is not enabled on the new API but it is rather easy to do so -- this call has no current existing users upstream though. Support will be provided once we add a respective series of test cases against it and find a proper upstream user for it.

The flexible API also adds a few new bells and whistles:

- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the requirements params. The new driver data API is able to free the driver data file for you by requiring a consumer callback for the driver data file.
- Allows both asynchronous and synchronous request to specify that driver data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- the driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- A firmware API framework is provided to enable daisy chaining a series of requests for firmware on a range of supported APIs.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 357 +++++++++++++++++++++
 include/linux/driver_data.h                        | 202 +++++++++++-
 include/linux/firmware.h                           |   2 +
 7 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst

Documentation/driver-api/firmware/driver_data.rst line 5

Quoted 11 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

new file mode 100644
index 000000000000..08407b7568fe
@@ -0,0 +1,77 @@
+===============
+driver_data API
+===============
+
+Users of firmware request APIs has grown to include users which are not

Grammar. Maybe "The usage of firmware..."

Documentation/driver-api/firmware/driver_data.rst line 10

Quoted 5 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+looking for "firmware", but instead general driver data files which have
+been kept oustide of the kernel. The driver data APIs addresses rebranding
+of firmware as generic driver data files, and provides a flexible API which
+mitigates collateral evolutions on the kernel as new functionality is
+introduced.

This looks more like a commit message than an introduction to the feature. In the future, we won't care why this was introduced, but we want to know what it is and how it can be used.

Documentation/driver-api/firmware/driver_data.rst line 15

Quoted 5 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for driver data requests:

"only" seems irrelevant here.

Quoted 8 lines

Documentation/driver-api/firmware/driver_data.rst new file +8 -0

+
+  * synchronous  - driver_data_request()
+  * asynchronous - driver_data_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters

Documentation/driver-api/firmware/driver_data.rst line 28

Quoted 5 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure is expected to grow as
+new requirements grow.

Again, not sure it's relevant to know that it can grow. For documentation purposes, the important is the *now*.

Quoted 12 lines

Documentation/driver-api/firmware/driver_data.rst new file +12 -0

+
+Reference counting and releasing the driver data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the driver data requests. This prevents removal
+of the device and module making the driver data request call until the
+driver 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.

Documentation/driver-api/firmware/driver_data.rst line 45

Quoted 5 lines

Documentation/driver-api/firmware/driver_data.rst new file +5 -0

+The driver data request API currently mimics this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs. In the future we may enable the ability to also refcount the
+caller's module as well. Likewise in the future we may enable asynchronous
+calls to refcount the firmware_class module.

Ditto. Maybe you could move all the "future" references to the [...]

Quoted 4 lines

drivers/base/firmware_class.c +0 -0

index f702566554e1..cc3c2247980c 100644

[...]

Quoted 14 lines

drivers/base/firmware_class.c +10 -0

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static int _driver_data_request_api(struct driver_data_params *params,
+				    struct device *device,
+				    const char *name)
+{
+	struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+	int ret;
+	char *try_name;
+	u8 api_max;
+

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1489

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+	if (priv_params->retry_api) {
+		if (!priv_params->api)
+			return -ENOENT;
+		api_max = priv_params->api - 1;
+	} else
+		api_max = req_params->api_max;

Braces.

Quoted 44 lines

drivers/base/firmware_class.c +44 -0

+	for (priv_params->api = api_max;
+	     priv_params->api >= req_params->api_min;
+	     priv_params->api--) {
+		if (req_params->api_name_postfix)
+			try_name = kasprintf(GFP_KERNEL, "%s%d%s",
+					     name,
+					     priv_params->api,
+					     req_params->api_name_postfix);
+		else
+			try_name = kasprintf(GFP_KERNEL, "%s%d",
+					     name,
+					     priv_params->api);
+		if (!try_name)
+			return -ENOMEM;
+		ret = _request_firmware(&params->driver_data, try_name,
+					params, device);
+		kfree(try_name);
+
+		if (!ret)
+			break;
+
+		release_firmware(params->driver_data);
+
+		/*
+		 * Only chug on with the API revision hunt if the file we
+		 * looked for really was not present. In case of memory issues
+		 * or other related system issues we want to bail right away
+		 * to not put strain on the system.
+		 */
+		if (ret != -ENOENT)
+			break;
+
+		if (!priv_params->api)
+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * driver_data_request - synchronous request for a driver data file
+ * @name: name of the driver data file
+ * @params: driver data parameters, it provides all the requirements
+ *	parameters which must be met for the file being requested.

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw)

Quoted 6 lines

drivers/base/firmware_class.c +6 -0

+ * @device: device for which firmware is being loaded
+ *
+ * This performs a synchronous driver data lookup with the requirements
+ * specified on @params, if the file was found meeting the criteria requested
+ * 0 is returned. Access to the driver data data can be accessed through
+ * an optional callback set on the @desc.

Huh? This last sentence seems wrong, I don't even see a @desc anywhere.

Quoted 26 lines

drivers/base/firmware_class.c +25 -0

 If the driver data is optional
+ * you must specify that on @params and if set you may provide an alternative
+ * callback which if set would be run if the driver data was not found.
+ *
+ * The driver data passed to the callbacks will be NULL unless it was
+ * found matching all the criteria on @params. 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 params->keep was set the kernel will
+ * release the driver data 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 driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;
+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1569

Quoted 4 lines

drivers/base/firmware_class.c +4 -0

+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;

Why do you need to check this here? If the caller is calling _sync(), it's because that's what it needs. This mode value here seems redundant.

OTOH, if you do have a reason for this value, then you could use driver_data_request_sync() in this if.

Quoted 14 lines

Quoted 14 lines

drivers/base/firmware_class.c +0 -0

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
   }
   EXPORT_SYMBOL(release_firmware);

drivers/base/firmware_class.c +10 -0

+static int _driver_data_request_api(struct driver_data_params *params,
+                               struct device *device,
+                               const char *name)
+{
+   struct driver_data_priv_params *priv_params = &params->priv_params;
+   const struct driver_data_req_params *req_params = &params->req_params;
+   int ret;
+   char *try_name;
+   u8 api_max;
+

Quoted 8 lines

Quoted 6 lines

+   if (priv_params->retry_api) {
+           if (!priv_params->api)
+                   return -ENOENT;
+           api_max = priv_params->api - 1;
+   } else
+           api_max = req_params->api_max;

Braces.

Not sure what you mean here, the else is a 1 liner so I skip the brace.

Quoted 23 lines · Luis R. Rodriguez

Quoted 14 lines

Quoted 14 lines

drivers/base/firmware_class.c +0 -0

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
   }
   EXPORT_SYMBOL(release_firmware);

drivers/base/firmware_class.c +10 -0

+static int _driver_data_request_api(struct driver_data_params *params,
+                               struct device *device,
+                               const char *name)
+{
+   struct driver_data_priv_params *priv_params = &params->priv_params;
+   const struct driver_data_req_params *req_params = &params->req_params;
+   int ret;
+   char *try_name;
+   u8 api_max;
+

Quoted 8 lines

Quoted 6 lines

+   if (priv_params->retry_api) {
+           if (!priv_params->api)
+                   return -ENOENT;
+           api_max = priv_params->api - 1;
+   } else
+           api_max = req_params->api_max;

Braces.

Quoted 1 line

Not sure what you mean here, the else is a 1 liner so I skip the brace.

It's really a nitpick, but the CodingStyle[1] says:

"Do not unnecessarily use braces where a single statement will do. [...]
This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches:

	if (condition) {
		do_this();
		do_that();
	} else {
		otherwise();
	}"

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n175

Quoted 12 lines

Quoted 12 lines

drivers/base/firmware_class.c +12 -0

+int driver_data_request_sync(const char *name,
+                        const struct driver_data_req_params *req_params,
+                        struct device *device)
+{
+   const struct firmware *driver_data;
+   const struct driver_data_reqs *sync_reqs;
+   struct driver_data_params params = {
+           .req_params = *req_params,
+   };
+   int ret;
+
+   if (!device || !req_params || !name || name[0] == '\0')

Quoted 8 lines

Quoted 4 lines

+           return -EINVAL;
+
+   if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC) +           return -EINVAL;

Why do you need to check this here? If the caller is calling _sync(), it's because that's what it needs.  This mode value here seems redundant.

Because we allow one data structure to be used for the specified requirements and technically someone can make a mistake and use the wrong macros to set up the data structure. This ensures users don't async macros to set up the parameters and then use the sync call. Eventually the underlying firmware_class.c code does its own conditional checks on this as well.

Quoted 2 lines

OTOH, if you do have a reason for this value, then you could use driver_data_request_sync() in this if.

You mean to allow *one* API call for all. Sure, that's possible, but I think its nicer to at least expose async/sync mechanisms on the caller side. The sync/async differences seem like a reasonable enough place to split the API.

Quoted 21 lines · Luis R. Rodriguez

Quoted 12 lines

Quoted 12 lines

drivers/base/firmware_class.c +12 -0

+int driver_data_request_sync(const char *name,
+                        const struct driver_data_req_params *req_params,
+                        struct device *device)
+{
+   const struct firmware *driver_data;
+   const struct driver_data_reqs *sync_reqs;
+   struct driver_data_params params = {
+           .req_params = *req_params,
+   };
+   int ret;
+
+   if (!device || !req_params || !name || name[0] == '\0')

Quoted 8 lines

Quoted 4 lines

+           return -EINVAL;
+
+   if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC) +           return -EINVAL;

Why do you need to check this here? If the caller is calling _sync(), it's because that's what it needs.  This mode value here seems redundant.

Quoted 5 lines

Because we allow one data structure to be used for the specified requirements and technically someone can make a mistake and use the wrong macros to set up the data structure. This ensures users don't async macros to set up the parameters and then use the sync call. Eventually the underlying firmware_class.c code does its own conditional checks on this as well.

If this could only happen in a programming error, maybe it's worth a WARN() then, to make it easier to spot?

Quoted 6 lines

Quoted 2 lines

OTOH, if you do have a reason for this value, then you could use driver_data_request_sync() in this if.

You mean to allow *one* API call for all. Sure, that's possible, but I think its nicer to at least expose async/sync mechanisms on the caller side. The sync/async differences seem like a reasonable enough place to split the API.

I don't remember the details of this anymore, but doesn't the driver_data_sync() function does exactly the same check? I meant that you could do this:

	if(WARN_ON_ONCE(!driver_data_request_sync()))
		return -EINVAL;

And yes, I think either a single API call for all or not having the mode in the struct would be cleaner. I think there are better ways to prevent coding errors (since this should only happen if the user code is implemented incorrectly).

The firmware API does not scale well: when new features are added we either add a new exported symbol or extend the arguments of existing routines. For the later case this 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". There are other subsystems which would like to make use of the firmware APIs for similar things and its clearly not firmware, but have different requirements and criteria which they'd like to be met for the requested file.

An extensible API is in order:

The driver data API accepts that there are only two types of requests:

a) synchronous requests
b) asynchronous requests

Both requests may have a different requirements which must be met. These requirements can be described in the struct driver_data_req_params. This struct is expected to be extended over time to support different requirements as the kernel evolves.

After a bit of hard work the new interface has been wrapped onto the functionality. The fallback mechanism has been kept out of the new API currently because it requires just a bit more grooming and documentation given new considerations and requirements. Adding support for it will be rather easy now that the new API sits ontop of the old one. The request_firmware_into_buf() API also is not enabled on the new API but it is rather easy to do so -- this call has no current existing users upstream though. Support will be provided once we add a respective series of test cases against it and find a proper upstream user for it.

The flexible API also adds a few new bells and whistles:

- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the requirements params. The new driver data API is able to free the driver data file for you by requiring a consumer callback for the driver data file.
- Allows both asynchronous and synchronous request to specify that driver data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- the driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- A firmware API framework is provided to enable daisy chaining a series of requests for firmware on a range of supported APIs.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +

 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 357 +++++++++++++++++++++
 include/linux/driver_data.h                        | 202 +++++++++++-
 include/linux/firmware.h                           |   2 +
 7 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst

Documentation/driver-api/firmware/driver_data.rst new file +77 -0

new file mode 100644
index 000000000000..08407b7568fe
@@ -0,0 +1,77 @@
+===============
+driver_data API
+===============
+
+Users of firmware request APIs has grown to include users which are not
+looking for "firmware", but instead general driver data files which have
+been kept oustide of the kernel. The driver data APIs addresses rebranding
+of firmware as generic driver data files, and provides a flexible API which
+mitigates collateral evolutions on the kernel as new functionality is
+introduced.
+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for driver data requests:
+
+  * synchronous  - driver_data_request()
+  * asynchronous - driver_data_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure is expected to grow as
+new requirements grow.
+
+Reference counting and releasing the driver data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the driver data requests. This prevents removal
+of the device and module making the driver data request call until the
+driver 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 driver data request API currently mimics this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs. In the future we may enable the ability to also refcount the
+caller's module as well. Likewise in the future we may enable asynchronous
+calls to refcount the firmware_class module.
+
+Typical use of the old synchronous firmware APIs consist of the caller
+requesting for "driver data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "driver data" and then finally freeing it on
+asynchronous callback.
+
+The driver 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 driver data around after your callbacks
+you must specify this through the driver data request parameter data structure.
+
+Synchronizing with async cookies
+================================
+
+The driver data API relies on async cookies to enable users to synchronize
+for any pending async work. The async cookie obtained through an async
+call using driver_data_file_request_async() can be used to synchronize and
+wait for pending work with driver_data_synchronize_request().
+
+When driver_data_file_request_async() completes you can rest assured all the
+work for both triggering, and processing the driver data using any of your
+callbacks has completed.
+
+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

Documentation/driver-api/firmware/index.rst +1 -0

index 1abe01793031..c2be92e2628c 100644
@@ -7,6 +7,7 @@ Linux Firmware API
    introduction
    core
    request_firmware
+   driver_data
 
 .. only::  subproject and html
 

Documentation/driver-api/firmware/introduction.rst +0 -0

index 211cb44eb972..a0f6a3fa1d5d 100644
@@ -25,3 +25,19 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time

Documentation/driver-api/firmware/introduction.rst +8 -0

+
+We have historically extended the firmware API by adding new routines or at
+times extending 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 driver data API is an extensible API enabling extensions to
+be added by avoiding unnecessary collateral evolutions as features get added.
+New features and development should be added through the driver_data API.

MAINTAINERS +2 -1

index 939311d601b8..3f025f738600 100644
@@ -5166,13 +5166,14 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, driver_data)
 M:	Luis R. Rodriguez <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/driver_data.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <[email protected]>

drivers/base/firmware_class.c +112 -0

index f702566554e1..cc3c2247980c 100644
@@ -2,6 +2,7 @@
  * firmware_class.c - Multi purpose firmware loading support
  *
  * Copyright (c) 2003 Manuel Estrada Sainz
+ * Copyright (c) 2017 Luis R. Rodriguez <[email protected]>
  *
  * Please see Documentation/firmware_class/ for more information.
  *

@@ -41,12 +42,20 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+static const struct driver_data_reqs dfl_sync_reqs = {
+	.mode = DRIVER_DATA_SYNC,
+	.module = THIS_MODULE,
+	.gfp = GFP_KERNEL,
+};
+
 struct driver_data_priv_params {
 	bool use_fallback;
 	bool use_fallback_uevent;
 	bool no_cache;
 	void *alloc_buf;
 	size_t alloc_buf_size;
+	u8 api;
+	bool retry_api;
 };
 
 struct driver_data_params {

@@ -1262,6 +1271,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 			  struct device *device,
 			  struct driver_data_params *data_params)
 {
+	struct driver_data_priv_params *priv_params = &data_params->priv_params;
 	struct firmware *firmware;
 	struct firmware_buf *buf;
 	int ret;

@@ -1285,6 +1295,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	 * of requesting firmware.
 	 */
 	firmware->priv = buf;
+	firmware->api = priv_params->api;
 
 	if (ret > 0) {
 		ret = fw_state_wait(&buf->fw_st);

@@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static int _driver_data_request_api(struct driver_data_params *params,
+				    struct device *device,
+				    const char *name)
+{
+	struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+	int ret;
+	char *try_name;
+	u8 api_max;
+
+	if (priv_params->retry_api) {
+		if (!priv_params->api)
+			return -ENOENT;
+		api_max = priv_params->api - 1;
+	} else
+		api_max = req_params->api_max;
+
+	for (priv_params->api = api_max;
+	     priv_params->api >= req_params->api_min;
+	     priv_params->api--) {
+		if (req_params->api_name_postfix)
+			try_name = kasprintf(GFP_KERNEL, "%s%d%s",
+					     name,
+					     priv_params->api,
+					     req_params->api_name_postfix);
+		else
+			try_name = kasprintf(GFP_KERNEL, "%s%d",
+					     name,
+					     priv_params->api);
+		if (!try_name)
+			return -ENOMEM;
+		ret = _request_firmware(&params->driver_data, try_name,
+					params, device);
+		kfree(try_name);
+
+		if (!ret)
+			break;
+
+		release_firmware(params->driver_data);
+
+		/*
+		 * Only chug on with the API revision hunt if the file we
+		 * looked for really was not present. In case of memory issues
+		 * or other related system issues we want to bail right away
+		 * to not put strain on the system.
+		 */
+		if (ret != -ENOENT)
+			break;
+
+		if (!priv_params->api)
+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * driver_data_request - synchronous request for a driver data file
+ * @name: name of the driver data file
+ * @params: driver data parameters, it provides all the requirements
+ *	parameters which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ *
+ * This performs a synchronous driver data lookup with the requirements
+ * specified on @params, if the file was found meeting the criteria requested
+ * 0 is returned. Access to the driver data data can be accessed through
+ * an optional callback set on the @desc. If the driver data is optional
+ * you must specify that on @params and if set you may provide an alternative
+ * callback which if set would be run if the driver data was not found.
+ *
+ * The driver data passed to the callbacks will be NULL unless it was
+ * found matching all the criteria on @params. 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 params->keep was set the kernel will
+ * release the driver data 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 driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;
+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;
+
+	if (driver_data_sync_opt_cb(req_params) &&
+	    !driver_data_param_optional(req_params))
+		return -EINVAL;
+

drivers/base/firmware_class.c +234 -0

+	if (!driver_data_param_keep(req_params))
+		release_firmware(driver_data);
+
+	put_device(device);
+	module_put(sync_reqs->module);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(driver_data_request_sync);
+
 /* Async support */
 struct firmware_work {
 	struct work_struct work;

@@ -1553,6 +1686,230 @@ request_firmware_nowait(
 }
 EXPORT_SYMBOL(request_firmware_nowait);
 
+static int validate_api_versioning(struct driver_data_params *params)
+{
+	//struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+
+	if (!req_params->api_max)
+		return -EINVAL;
+	if (req_params->api_max < req_params->api_min)
+		return -EINVAL;
+	/*
+	 * this is being processed, but the first iteration this check is invalid
+	 */
+		 /*
+	if (priv_params->api < req_params->api_min)
+		return -EINVAL;
+		*/
+	if (driver_data_async_cb(req_params))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int __request_driver_data_api(struct firmware_work *driver_work)
+{
+	struct driver_data_params *params = &driver_work->data_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+	int ret;
+
+	ret = _driver_data_request_api(params, driver_work->device,
+				       driver_work->name);
+	/*
+	 * The default async optional callbacks don't work well here, so they
+	 * are not supported. Consider what we would return back, we'd need a
+	 * non-void optional callback to be able to feed the consumer.
+	 */
+	if (ret == -ENOENT)
+		dev_err(driver_work->device,
+			"No API file in range %u - %u could be found\n",
+			req_params->api_min, req_params->api_max);
+
+	return driver_data_async_call_api_cb(params->driver_data, req_params);
+}
+
+static void request_driver_data_single(struct firmware_work *driver_work)
+{
+	struct driver_data_params *params = &driver_work->data_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+	const struct driver_data_reqs *sync_reqs;
+	int ret;
+
+	sync_reqs = &req_params->sync_reqs;
+
+	ret = _request_firmware(&params->driver_data, driver_work->name,
+				params, driver_work->device);
+	if (ret &&
+	    driver_data_param_optional(req_params) &&
+	    driver_data_async_opt_cb(req_params))
+		driver_data_async_opt_call_cb(req_params);
+	else
+		driver_data_async_call_cb(params->driver_data, req_params);
+
+	if (!driver_data_param_keep(req_params))
+		release_firmware(params->driver_data);
+
+	put_device(driver_work->device);
+	module_put(sync_reqs->module);
+
+	kfree_const(driver_work->name);
+	kfree(driver_work);
+}
+
+/*
+ * Instead of recursion provide a deterministic limit based on the parameters,
+ * and consume less memory.
+ */
+static void request_driver_data_api(struct firmware_work *driver_work)
+{
+	struct driver_data_params *params = &driver_work->data_params;
+	struct driver_data_priv_params *priv_params = &params->priv_params;
+	const struct driver_data_req_params *req_params = &params->req_params;
+	const struct driver_data_reqs *sync_reqs;
+	int ret;
+	u8 i, limit;
+
+	limit = req_params->api_max - req_params->api_min;
+	sync_reqs = &req_params->sync_reqs;
+
+	for (i=0; i <= limit; i++) {
+		ret = validate_api_versioning(params);
+		if (WARN(ret, "Invalid API driver data request")) {
+			ret = driver_data_async_call_api_cb(NULL, req_params);
+			goto out;
+		}
+
+		/*
+		 * This does the real work of fetching the driver data through
+		 * all the API revisions possible. If found the api and its
+		 * return value are passed. If a value of 0 is passed then
+		 * *really* does mean everything was peachy. If we catch
+		 * -EAGAIN here it means the driver's API callback asked us to
+		 * try again.
+		 */
+		ret = __request_driver_data_api(driver_work);
+		if (!ret)
+			break;
+
+		priv_params->retry_api = true;
+
+		release_firmware(params->driver_data);
+
+		if (ret != -EAGAIN)
+			break;
+	}
+
+	if (ret && driver_data_param_optional(req_params))
+		driver_data_async_opt_call_cb(req_params);
+
+	if (!driver_data_param_keep(req_params))
+		release_firmware(params->driver_data);
+
+out:
+	put_device(driver_work->device);
+	module_put(sync_reqs->module);
+
+	kfree_const(driver_work->name);
+	kfree(driver_work);
+}
+
+static void request_driver_data_work_func(struct work_struct *work)
+{
+	struct firmware_work *driver_work;
+	struct driver_data_params *data_params;
+	const struct driver_data_req_params *req_params;
+
+	driver_work = container_of(work, struct firmware_work, work);
+	data_params = &driver_work->data_params;
+	req_params = &data_params->req_params;
+
+	if (driver_data_param_uses_api(req_params))
+		request_driver_data_api(driver_work);
+	else
+		request_driver_data_single(driver_work);
+}
+
+/**
+ * driver_data_request_async - asynchronous request for a driver data file
+ * @name: name of the driver data file
+ * @req_params: driver data file request parameters, 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 driver data file lookup with the requirements
+ * specified on @req_params. The request for the actual driver data file lookup
+ * will be scheduled with schedule_work() to be run at a later time. 0 is
+ * returned if we were able to asynchronously schedlue your 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 driver data file data can be accessed through an optional
+ * callback set on the @req_params. If the driver data file is optional you
+ * must specify that on @req_params and if set you may provide an alternative
+ * callback which if set would be run if the driver data file was not found.
+ *
+ * The driver data file passed to the callbacks will always be NULL unless it
+ * was found matching all the criteria on @req_params. Unless the desc->keep
+ * was set the kernel will release the driver data file for you after your
+ * callbacks were processed on the scheduled work.
+ */
+int driver_data_request_async(const char *name,
+			      const struct driver_data_req_params *req_params,
+			      struct device *device)
+{
+	struct firmware_work *driver_work;
+	const struct driver_data_reqs *sync_reqs;
+	struct firmware_work driver_work_stack = {
+		.data_params.req_params = *req_params,
+		//.device = device,
+		//.name = name,
+	};
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_ASYNC)
+		return -EINVAL;
+
+	if (driver_data_async_opt_cb(req_params) && !req_params->optional)
+		return -EINVAL;
+
+	sync_reqs = &req_params->sync_reqs;
+
+	driver_work = kzalloc(sizeof(struct firmware_work), sync_reqs->gfp);
+	if (!driver_work)
+		return -ENOMEM;
+
+	//memcpy(&driver_work->params, &driver_work_stack.params,
+	 //      sizeof(struct driver_data_file_work));
+	memcpy(driver_work, &driver_work_stack, sizeof(struct firmware_work));
+
+	driver_work->name = kstrdup_const(name, sync_reqs->gfp);
+	if (!driver_work->name) {
+		kfree(driver_work);
+		return -ENOMEM;
+	}
+	driver_work->device = device;
+
+	if (!try_module_get(sync_reqs->module)) {
+		kfree_const(driver_work->name);
+		kfree(driver_work);
+		return -EFAULT;
+	}
+
+	get_device(driver_work->device);
+
+	INIT_WORK(&driver_work->work, request_driver_data_work_func);
+	schedule_work(&driver_work->work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(driver_data_request_async);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 

include/linux/driver_data.h +41 -0

index c3d3a4129838..fda1e716017d 100644
@@ -5,6 +5,7 @@
 #include <linux/compiler.h>
 #include <linux/gfp.h>
 #include <linux/device.h>
+#include <linux/firmware.h>
 
 /*
  * Driver Data internals

@@ -33,9 +34,25 @@ enum driver_data_mode {
 /* one per driver_data_mode */
 union driver_data_cbs {
 	struct {
+		int __must_check
+			(*found_cb)(void *context,
+				    const struct firmware *driver_data);
+		void *found_ctx;
+
+		int __must_check (*opt_fail_cb)(void *context);
+		void *opt_fail_ctx;
+	} sync;
+	struct {
 		void (*found_cb)(const struct firmware *driver_data,
 				 void *context);
 		void *found_ctx;
+
+		void (*opt_fail_cb)(void *context);
+		void *opt_fail_ctx;
+
+		int __must_check
+			(*found_api_cb)(const struct firmware *driver_data,
+					void *context);
 	} async;
 };
 

@@ -50,6 +67,30 @@ struct driver_data_reqs {
  * @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 driver data
+ *	through one of its callbacks, it must later free it with
+ *	release_driver_data(). By default this is set to false and the kernel
+ *	will release the driver data file for you after callback processing
+ *	has completed.
+ * @uses_api_versioning: if set the caller is indicating that the caller has
+ * 	an API revision system for the the files being requested using a
+ * 	simple numeric scheme: there is a max API version supported and the
+ * 	lowest API version supported. When used the driver data request request
+ * 	will always look for the filename requested but by first appeneding the
+ * 	@api_max to it, and looking for that. If that is not available it will
+ * 	look for any files with API version lower than this until it reaches
+ * 	@api_min. This enables chaining driver data requests easily on behalf
+ * 	of device drivers using a simple one digit versioning scheme. When
+ * 	this is true it will treat all files not found as non-fatal, as
+ * 	optional, but it requires at least one file to be found. If the
+ * 	@optional attribute is also true then if no files are found we won't
+ * 	complain at all.
+ * @api_min: if uses_api_versioning is set, this represents the lowest
+ *	version of API supported by the caller.
+ * @api_max: if uses_api_versioning is set, this represents the highest
+ *	version of API supported by the caller.
+ * @api_name_postfix: use the name as the driver data name prefix, the API
+ *	digit will be placed in the middle, followed by the @api_name_postfix.
  * @sync_reqs: synchronization requirements
  *
  * This data structure is intended to carry all requirements and specifications

include/linux/driver_data.h +158 -1

+	u8 api_min;
+	u8 api_max;
+	const char *api_name_postfix;
 	struct driver_data_reqs sync_reqs;
 	const union driver_data_cbs cbs;
 };
 
+/*
+ * We keep these template definitions to a minimum for the most
+ * popular requests.
+ */
+
+/* Typical sync data case */
+#define DRIVER_DATA_SYNC_FOUND(__found_cb, __ctx)			\
+	.cbs.sync.found_cb = __found_cb,				\
+	.cbs.sync.found_ctx = __ctx
+
+#define DRIVER_DATA_DEFAULT_SYNC(__found_cb, __ctx)			\
+	DRIVER_DATA_SYNC_FOUND(__found_cb, __ctx)
+
+#define DRIVER_DATA_KEEP_SYNC(__found_cb, __ctx)			\
+	DRIVER_DATA_DEFAULT_SYNC(__found_cb, __ctx),			\
+	.keep = true
+
+/* If you have one fallback routine */
+#define DRIVER_DATA_SYNC_OPT_CB(__fail_cb, __ctx)			\
+	.optional = true,						\
+	.cbs.sync.opt_fail_cb = __fail_cb,				\
+	.cbs.sync.opt_fail_ctx = __ctx
+
+/*
+ * Used to define the default asynchronization requirements for
+ * driver_data_request_async(). Drivers can override.
+ */
+#define DRIVER_DATA_DEFAULT_ASYNC(__found_cb, __ctx)			\
+	.sync_reqs = {							\
+		.mode = DRIVER_DATA_ASYNC,				\
+		.module = THIS_MODULE,					\
+		.gfp = GFP_KERNEL,					\
+	},								\
+	.cbs.async = {							\
+		.found_cb = __found_cb,					\
+		.found_ctx = __ctx,					\
+	}
+
+#define DRIVER_DATA_DEFAULT_ASYNC_OPT(__found_cb, __ctx)		\
+	DRIVER_DATA_DEFAULT_ASYNC(__found_cb, __ctx),			\
+	.optional = true
+
+#define DRIVER_DATA_KEEP_ASYNC(__found_cb, __ctx)			\
+	DRIVER_DATA_DEFAULT_ASYNC(__found_cb, __ctx),			\
+	.keep = true
+
+#define DRIVER_DATA_KEEP_ASYNC_OPT(__found_cb, __ctx)			\
+	DRIVER_DATA_DEFAULT_ASYNC(__found_cb, __ctx),			\
+	.optional = true
+
+#define DRIVER_DATA_ASYNC_OPT_CB(__fail_cb, __ctx)			\
+	.optional = true,						\
+	.cbs.async.opt_fail_cb = __fail_cb,				\
+	.cbs.async.opt_fail_ctx = __ctx
+
+#define DRIVER_DATA_API_CB(__found_api_cb, __ctx)			\
+	.sync_reqs = {							\
+		.mode = DRIVER_DATA_ASYNC,				\
+		.module = THIS_MODULE,					\
+		.gfp = GFP_KERNEL,					\
+	},								\
+	.cbs.async = {							\
+		.found_api_cb = __found_api_cb,				\
+		.found_ctx = __ctx,					\
+	}
+
+#define DRIVER_DATA_API(__min, __max, __postfix)			\
+	.uses_api_versioning = true,					\
+	.api_min = __min,						\
+	.api_max = __max,						\
+	.api_name_postfix = __postfix
+
 #define driver_data_req_param_sync(params)				\
 	((params)->sync_reqs.mode == DRIVER_DATA_SYNC)
 #define driver_data_req_param_async(params)				\
 	((params)->sync_reqs.mode == DRIVER_DATA_ASYNC)
 
 #define driver_data_param_optional(params)	((params)->optional)
+#define driver_data_param_keep(params)		((params)->keep)
+#define driver_data_param_uses_api(params)	((params)->uses_api_versioning)
+
+#define driver_data_sync_cb(param)   ((params)->cbs.sync.found_cb)
+#define driver_data_sync_ctx(params) ((params)->cbs.sync.found_ctx)
+static inline
+int driver_data_sync_call_cb(const struct driver_data_req_params *params,
+			     const struct firmware *driver_data)
+{
+	if (!driver_data_req_param_sync(params))
+		return -EINVAL;
+	if (!driver_data_sync_cb(params)) {
+		if (driver_data)
+			return 0;
+		return -ENOENT;
+	}
+	return driver_data_sync_cb(params)(driver_data_sync_ctx(params),
+					   driver_data);
+}
+
+#define driver_data_sync_opt_cb(params)  ((params)->cbs.sync.opt_fail_cb)
+#define driver_data_sync_opt_ctx(params) ((params)->cbs.sync.opt_fail_ctx)
+static inline
+int driver_data_sync_opt_call_cb(const struct driver_data_req_params *params)
+{
+	if (params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;
+	if (!driver_data_sync_opt_cb(params))
+		return 0;
+	return driver_data_sync_opt_cb(params)
+		(driver_data_sync_opt_ctx(params));
+}
 
 #define driver_data_async_cb(params)		((params)->cbs.async.found_cb)
 #define driver_data_async_ctx(params)		((params)->cbs.async.found_ctx)
-
 static inline
 void driver_data_async_call_cb(const struct firmware *driver_data,
 			       const struct driver_data_req_params *params)

@@ -85,4 +234,55 @@ void driver_data_async_call_cb(const struct firmware *driver_data,
 				     driver_data_async_ctx(params));
 }
 
+#define driver_data_async_opt_cb(params)  ((params)->cbs.async.opt_fail_cb)
+#define driver_data_async_opt_ctx(params) ((params)->cbs.async.opt_fail_ctx)
+static inline
+void driver_data_async_opt_call_cb(const struct driver_data_req_params *params)
+{
+	if (params->sync_reqs.mode != DRIVER_DATA_ASYNC)
+		return;
+	if (!driver_data_async_opt_cb(params))
+		return;
+	driver_data_async_opt_cb(params)(driver_data_async_opt_ctx(params));
+}
+
+#define driver_data_async_api_cb(params)	((params)->cbs.async.found_api_cb)
+static inline
+int driver_data_async_call_api_cb(const struct firmware *driver_data,
+				  const struct driver_data_req_params *params)
+{
+	if (!params->uses_api_versioning)
+		return -EINVAL;
+	if (!driver_data_async_api_cb(params))
+		return -EINVAL;
+	return driver_data_async_api_cb(params)(driver_data,
+						driver_data_async_ctx(params));
+}
+
+#if defined(CONFIG_FW_LOADER) || \
+	(defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+int driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *params,
+			     struct device *device);
+int driver_data_request_async(const char *name,
+			      const struct driver_data_req_params *params,
+			      struct device *device);
+#else
+static
+inline int driver_data_request_sync(const char *name,
+				    const struct driver_data_req_params *params,
+				    struct device *device)
+{
+	return -EINVAL;
+}
+
+static
+inline int driver_data_request_async(const char *name,
+				 const struct driver_data_req_params *params,
+				 struct device *device)
+{
+	return -EINVAL;
+}
+#endif
+
 #endif /* _LINUX_DRIVER_DATA_H */

include/linux/firmware.h +2 -0

index b1f9f0ccb8ac..3a71924d35d7 100644
@@ -13,6 +13,8 @@ struct firmware {
 	const u8 *data;
 	struct page **pages;
 
+	u8 api;
+
 	/* firmware loader private fields */
 	void *priv;
 };

Signature · 1 line

2.11.0

Quoted 5 lines · Luis R. Rodriguez

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +

I think we'd better to split code and documents into different patches for easier reviews.

Quoted 4 lines

Documentation/driver-api/firmware/introduction.rst +0 -0

@@ -25,3 +25,19 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time

Documentation/driver-api/firmware/introduction.rst are already using asynchronous initialization mechanisms which will not · line 35

Quoted 10 lines

Documentation/driver-api/firmware/introduction.rst +8 -0

 processing firmware might, and this can still delay boot or initialization,
 as such mechanisms such as asynchronous probe can help supplement drivers.
+
+Two APIs
+========
+
+Two APIs are provided for firmware:
+
+* request_firmware API - old firmware API
+* driver_data API - flexible API

You can add links:

* request_firmware API_ - old firmware API
* driver_data API_ - flexible API

.. _request_firmware API: ./request_firmware.rst .. _driver_data API: ./driver_data.rst

Quoted 27 lines

Documentation/driver-api/firmware/introduction.rst +27 -0

+int driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;
+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;
+
+	if (driver_data_sync_opt_cb(req_params) &&
+	    !driver_data_param_optional(req_params))
+		return -EINVAL;
+
+	sync_reqs = &dfl_sync_reqs;
+
+	__module_get(sync_reqs->module);
+	get_device(device);
+
+	ret = _request_firmware(&driver_data, name, &params, device);

Documentation/driver-api/firmware/introduction.rst

Quoted 4 lines

Documentation/driver-api/firmware/introduction.rst +4 -0

+	if (ret && driver_data_param_optional(req_params))
+		ret = driver_data_sync_opt_call_cb(req_params);
+	else
+		ret = driver_data_sync_call_cb(req_params, driver_data);

It looks a bit weird to me that a failure callback is called only if "optional" is set. I think that it makes more sense that a failure callback is always called if _request_firmware() fails.

In addition, why not always return a return value of _request_firmare()? Overwriting a return value by any of callback functions doesn't make sense, particularly, in "sync" case.
One of the problems in this implementation is that we, drivers, have no chance to know a return value of _request_firmware().

For example, if the signature verification, which I'm now working on, fails, ENOKEY or EKEYxxx will be returns. We may want to say more detailed error messages depending on error code.

Documentation/driver-api/firmware/introduction.rst

Quoted 4 lines

struct driver_data_req_params {

Documentation/driver-api/firmware/introduction.rst +2 -0

 	bool optional;
+	bool keep;
+	bool uses_api_versioning;

Do you have any reason that you don't use bit fields here? More features are added, more 'boolean' are added. (I mean it wastes memory.)

Quoted 9 lines · AKASHI Takahiro

Quoted 5 lines · Luis R. Rodriguez

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +

I think we'd better to split code and documents into different patches for easier reviews.

Sure, done.

Quoted 17 lines

Quoted 4 lines

Documentation/driver-api/firmware/introduction.rst +0 -0

@@ -25,3 +25,19 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time

Documentation/driver-api/firmware/introduction.rst are already using asynchronous initialization mechanisms which will not · line 35

Quoted 10 lines

Documentation/driver-api/firmware/introduction.rst +8 -0

 processing firmware might, and this can still delay boot or initialization,
 as such mechanisms such as asynchronous probe can help supplement drivers.
+
+Two APIs
+========
+
+Two APIs are provided for firmware:
+
+* request_firmware API - old firmware API
+* driver_data API - flexible API

You can add links:

Quoted 5 lines

* request_firmware API_ - old firmware API
* driver_data API_ - flexible API

.. _request_firmware API: ./request_firmware.rst .. _driver_data API: ./driver_data.rst

Done!

Quoted 27 lines

Quoted 27 lines

Documentation/driver-api/firmware/introduction.rst +27 -0

+int driver_data_request_sync(const char *name,
+			     const struct driver_data_req_params *req_params,
+			     struct device *device)
+{
+	const struct firmware *driver_data;
+	const struct driver_data_reqs *sync_reqs;
+	struct driver_data_params params = {
+		.req_params = *req_params,
+	};
+	int ret;
+
+	if (!device || !req_params || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
+		return -EINVAL;
+
+	if (driver_data_sync_opt_cb(req_params) &&
+	    !driver_data_param_optional(req_params))
+		return -EINVAL;
+
+	sync_reqs = &dfl_sync_reqs;
+
+	__module_get(sync_reqs->module);
+	get_device(device);
+
+	ret = _request_firmware(&driver_data, name, &params, device);

Quoted 8 lines

Documentation/driver-api/firmware/introduction.rst

Quoted 4 lines

Documentation/driver-api/firmware/introduction.rst +4 -0

+	if (ret && driver_data_param_optional(req_params))
+		ret = driver_data_sync_opt_call_cb(req_params);
+	else
+		ret = driver_data_sync_call_cb(req_params, driver_data);

It looks a bit weird to me that a failure callback is called only if "optional" is set. I think that it makes more sense that a failure callback is always called if _request_firmware() fails.

Let's think about this: does it make sense for the there to be a callback if the file was not optional ? Keep in mind the optional flag has its own semantics, it affects printing on error, on file not found. The semantics of the "optional callback" is precisely defined for when the first file is optional, so its by definition.

If we were to not require optional then it would be more of a "failure callback", as you put it, but then folks could be stuffing these with all their error paths, and that's not what this is for. The optional callback is to handle an alternative *viable* approach *iff* the first file we look for is not found.

Quoted 5 lines

In addition, why not always return a return value of _request_firmare()? Overwriting a return value by any of callback functions doesn't make sense, particularly, in "sync" case.
One of the problems in this implementation is that we, drivers, have no chance to know a return value of _request_firmware().

Ah, good point, well, we can pass it on the optional callback then, this way no information is lost.

Thoughts?

Quoted 3 lines

For example, if the signature verification, which I'm now working on, fails, ENOKEY or EKEYxxx will be returns. We may want to say more detailed error messages depending on error code.

Makes sense. If the above suffices let me know.

Quoted 8 lines

Documentation/driver-api/firmware/introduction.rst

Quoted 4 lines

struct driver_data_req_params {

Documentation/driver-api/firmware/introduction.rst +2 -0

 	bool optional;
+	bool keep;
+	bool uses_api_versioning;

Do you have any reason that you don't use bit fields here? More features are added, more 'boolean' are added. (I mean it wastes memory.)

You're right, will fold into a flags.

Luis

Quoted 16 lines

Quoted 9 lines

drivers/base/firmware_class.c void release_firmware(const struct firmware *fw) · line 1584

Quoted 5 lines

drivers/base/firmware_class.c +5 -0

+	ret = _request_firmware(&driver_data, name, &params, device);
+	if (ret && driver_data_param_optional(req_params))
+		ret = driver_data_sync_opt_call_cb(req_params);
+	else
+		ret = driver_data_sync_call_cb(req_params, driver_data);

It looks a bit weird to me that a failure callback is called only if "optional" is set. I think that it makes more sense that a failure callback is always called if _request_firmware() fails.

Let's think about this: does it make sense for the there to be a callback if the file was not optional ? Keep in mind the optional flag has its own semantics, it affects printing on error, on file not found. The semantics of the "optional callback" is precisely defined for when the first file is optional, so its by definition.

Quoted 4 lines

If we were to not require optional then it would be more of a "failure callback", as you put it, but then folks could be stuffing these with all their error paths, and that's not what this is for. The optional callback is to handle an alternative *viable* approach *iff* the first file we look for is not found.

In sync case, I don't think we have a strong reason to have a callback as we can do anything depending on a return value from _request_firmware(). The only merit would be that we could release buffers automatically?

In async case, I think that we should have a callback whether asynchronous loader has succeeded or failed in order to know the result.

It will never be "optional" even on failure.

Quoted 6 lines

Quoted 5 lines

In addition, why not always return a return value of _request_firmare()? Overwriting a return value by any of callback functions doesn't make sense, particularly, in "sync" case.
One of the problems in this implementation is that we, drivers, have no chance to know a return value of _request_firmware().

Quoted 4 lines

Ah, good point, well, we can pass it on the optional callback then, this way no information is lost.

Thoughts?

Depends on the discussion above.

This adds a load tester driver test_driver_data a for the new extensible driver_data loader API, part of firmware_class. This test driver enables you to build your tests in userspace by exposing knobs of the exported API to userspace and enables a trigger action to mimic a one time use of the kernel API. This gives us the flexibility to build test case from userspace with less kernel changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/driver-api/firmware/driver_data.rst |   32 +
 MAINTAINERS                                       |    1 +
 lib/Kconfig.debug                                 |   12 +
 lib/Makefile                                      |    1 +
 lib/test_driver_data.c                            | 1272 +++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile         |    2 +-
 tools/testing/selftests/firmware/config           |    1 +
 tools/testing/selftests/firmware/driver_data.sh   |  996 ++++++++++++++++
 8 files changed, 2316 insertions(+), 1 deletion(-)
 create mode 100644 lib/test_driver_data.c
 create mode 100755 tools/testing/selftests/firmware/driver_data.sh

Documentation/driver-api/firmware/driver_data.rst +5 -0

index 08407b7568fe..757c2ffa4ba6 100644
@@ -68,6 +68,38 @@ When driver_data_file_request_async() completes you can rest assured all the
 work for both triggering, and processing the driver data using any of your
 callbacks has completed.
 
+Testing the driver_data API
+===========================
+
+The driver data API has a selftest driver: lib/test_driver_data.c. The
+test_driver_data enables you to build your tests in userspace by exposing knobs

Documentation/driver-api/firmware/driver_data.rst +17 -0

+        echo -n 1 >  /sys/devices/virtual/misc/test_driver_data0/config_async
+        echo -n 1 >  /sys/devices/virtual/misc/test_driver_data0/trigger_config
+
+A series of tests have been written to test the driver data API thoroughly.
+A respective test case is expected to bet written as new features get added.
+For details of existing tests run::
+
+        tools/testing/selftests/firmware/driver_data.sh -l
+
+To see all available options::
+
+        tools/testing/selftests/firmware/driver_data.sh --help
+
+To run a test 0010 case 40 times::
+
+        tools/testing/selftests/firmware/driver_data.sh -c 0010 40
+
 Tracking development enhancements and ideas
 ===========================================
 

MAINTAINERS +1 -0

index 3f025f738600..a0a81c245fb3 100644
@@ -5172,6 +5172,7 @@ L:	[email protected]
 S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
+F:	lib/test_driver_data.c
 F:	include/linux/firmware.h
 F:	include/linux/driver_data.h
 

lib/Kconfig.debug +12 -0

index 77fadface4f9..53dfd7db557b 100644
@@ -1964,6 +1964,18 @@ config TEST_FIRMWARE
 
 	  If unsure, say N.
 
+config TEST_DRIVER_DATA
+	tristate "Test driver data loading via driver_data APIs"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_driver_data" module that creates a userspace
+	  interface for testing driver data loading using the driver_data API.
+	  This can be used to control the triggering of driver data loading
+	  without needing an actual real device.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	default n

lib/Makefile +1 -0

index 0f64ef3956bf..d5042ad4dad9 100644
@@ -50,6 +50,7 @@ obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
+obj-$(CONFIG_TEST_DRIVER_DATA) += test_driver_data.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o

lib/test_driver_data.c new file +0 -0

new file mode 100644
index 000000000000..11175a3b9f0a

lib/test_driver_data.c new file +1264 -0

+ *
+ * This module provides an interface to trigger and test the driver data API
+ * through a series of configurations and a few triggers. This driver
+ * lacks any extra dependencies, and will not normally be loaded by the
+ * system unless explicitly requested by name. You can also build this
+ * driver into your kernel.
+ *
+ * Although all configurations are already written for and will be supported
+ * for this test driver, ideally we should strive to see what mechanisms we
+ * can put in place to instead automatically generate this sort of test
+ * interface, test cases, and infer results. Its a simple enough interface that
+ * should hopefully enable more exploring in this area.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/completion.h>
+#include <linux/driver_data.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/vmalloc.h>
+
+/* Used for the fallback default to test against */
+#define TEST_DRIVER_DATA "test-driver_data.bin"
+
+/*
+ * For device allocation / registration
+ */
+static DEFINE_MUTEX(reg_dev_mutex);
+static LIST_HEAD(reg_test_devs);
+
+/*
+ * num_test_devs actually represents the *next* ID of the next
+ * device we will allow to create.
+ */
+int num_test_devs;
+
+/**
+ * test_config - represents configuration for the driver_data API
+ *
+ * @name: the name of the primary driver_data file to look for
+ * @default_name: a fallback example, used to test the optional callback
+ *	mechanism.
+ * @async: true if you want to trigger an async request. This will use
+ *	driver_data_request_async(). If false the synchronous call will
+ *	be used, driver_data_request_sync().
+ * @optional: whether or not the driver_data is optional refer to the
+ *	struct driver_data_reg_params @optional field for more information.
+ * @keep: whether or not we wish to free the driver_data on our own, refer to
+ *	the struct driver_data_req_params @keep field for more information.
+ * @enable_opt_cb: whether or not the optional callback should be set
+ *	on a trigger. There is no equivalent setting on the struct
+ *	driver_data_req_params as this is implementation specific, and in
+ *	in driver_data API its explicit if you had defined an optional call
+ *	back for your descriptor with either DRIVER_DATA_SYNC_OPT_CB() or
+ *	DRIVER_DATA_ASYNC_OPT_CB(). Since the params are in a const we have
+ *	no option but to use a flag and two const structs to decide which
+ *	one we should use.
+ * @use_api_versioning: use the driver data API versioning support. This
+ *	currenlty implies you are using an async test.
+ * @api_min: API min version to use for the test.
+ * @api_max: API max version to use for the test.
+ * @api_name_postfix: API name postfix
+ * @test_result: a test may use this to collect the result from the call
+ *	of the driver_data_request_async() or driver_data_request_sync() calls
+ *	used in their tests. Note that for async calls this typically will be a
+ *	successful result (0) unless of course you've used bogus parameters, or
+ *	the system is out of memory. Tests against the callbacks can only be
+ *	implementation specific, so we don't test for that for now but it may
+ *	make sense to build tests cases against a series of semantically
+ *	similar family of callbacks that generally represents usage in the
+ *	kernel. Synchronous calls return bogus error checks against the
+ *	parameters as well, but also return the result of the work from the
+ *	callbacks. You can therefore rely on sync calls if you really want to
+ *	test for the callback results as well. Errors you can expect:
+ *
+ *	API specific:
+ *
+ *	0:		success for sync, for async it means request was sent
+ *	-EINVAL:	invalid parameters or request
+ *	-ENOENT:	files not found
+ *
+ *	System environment:
+ *
+ *	-ENOMEM:	memory pressure on system
+ *	-ENODEV:	out of number of devices to test
+ *
+ * The ordering of elements in this struct must match the exact order of the
+ * elements in the ATTRIBUTE_GROUPS(test_dev_config), this is done to know
+ * what corresponding field each device attribute configuration entry maps
+ * to what struct member on test_alloc_dev_attrs().
+ */
+struct test_config {
+	char *name;
+	char *default_name;
+	bool async;
+	bool optional;
+	bool keep;
+	bool enable_opt_cb;
+	bool use_api_versioning;
+	u8 api_min;
+	u8 api_max;
+	char *api_name_postfix;
+
+	int test_result;
+};
+
+/**
+ * test_driver_data_private - private device driver driver_data representation
+ *
+ * @size: size of the data copied, in bytes
+ * @data: the actual data we copied over from driver_data
+ * @written: true if a callback managed to copy data over to the device
+ *	successfully. Since different callbacks are used for this purpose
+ *	having the data written does not necessarily mean a test case
+ *	completed successfully. Each tests case has its own specific
+ *	goals.
+ *
+ * Private representation of buffer where we put the device system data.
+ */
+struct test_driver_data_private {
+	size_t size;
+	u8 *data;
+	u8 api;
+	bool written;
+};
+
+/**
+ * driver_data_test_device - test device to help test driver_data
+ *
+ * @dev_idx: unique ID for test device
+ * @config: this keeps the device's own configuration. Instead of creating
+ *	different triggers for all possible test cases we can think of in
+ *	kernel, we expose a set possible device attributes for tuning the
+ *	driver_data API and we to let you tune them in userspace. We then just
+ *	provide one trigger.
+ * @test_driver_data: internal private representation of a storage area
+ *	a driver might typically use to stuff firmware / driver_data.
+ * @misc_dev: we use a misc device under the hood
+ * @dev: pointer to misc_dev's own struct device
+ * @api_found_calls: number of calls a fetch for a driver was found. We use
+ *	for internal use on the api callback.
+ * @driver_data_mutex: for access into the @driver_data, the fake storage
+ *	location for the system data we copy.
+ * @config_mutex: used to protect configuration changes
+ * @trigger_mutex: all triggers are mutually exclusive when testing. To help
+ *	enable testing you can create a different device, each device has its
+ *	own set of protections, mimicking real devices.
+ * @request_complete: used to help the driver inform itself when async
+ * 	callbacks complete.
+ * list: needed to be part of the reg_test_devs
+ */
+struct driver_data_test_device {
+	int dev_idx;
+	struct test_config config;
+	struct test_driver_data_private test_driver_data;
+	struct miscdevice misc_dev;
+	struct device *dev;
+
+	u8 api_found_calls;
+
+	struct mutex driver_data_mutex;
+	struct mutex config_mutex;
+	struct mutex trigger_mutex;
+	struct completion request_complete;
+	struct list_head list;
+};
+
+static struct miscdevice *dev_to_misc_dev(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
+static struct driver_data_test_device *
+misc_dev_to_test_dev(struct miscdevice *misc_dev)
+{
+	return container_of(misc_dev, struct driver_data_test_device, misc_dev);
+}
+
+static struct driver_data_test_device *dev_to_test_dev(struct device *dev)
+{
+	struct miscdevice *misc_dev;
+
+	misc_dev = dev_to_misc_dev(dev);
+
+	return misc_dev_to_test_dev(misc_dev);
+}
+
+static ssize_t test_fw_misc_read(struct file *f, char __user *buf,
+				 size_t size, loff_t *offset)
+{
+	struct miscdevice *misc_dev = f->private_data;
+	struct driver_data_test_device *test_dev =
+		misc_dev_to_test_dev(misc_dev);
+	struct test_driver_data_private *test_driver_data =
+		&test_dev->test_driver_data;
+	ssize_t ret = 0;
+
+	mutex_lock(&test_dev->driver_data_mutex);
+	if (test_driver_data->written)
+		ret = simple_read_from_buffer(buf, size, offset,
+					      test_driver_data->data,
+					      test_driver_data->size);
+	mutex_unlock(&test_dev->driver_data_mutex);
+
+	return ret;
+}
+
+static const struct file_operations test_fw_fops = {
+	.owner          = THIS_MODULE,
+	.read           = test_fw_misc_read,
+};
+
+static
+void free_test_driver_data(struct test_driver_data_private *test_driver_data)
+{
+	kfree(test_driver_data->data);
+	test_driver_data->data = NULL;
+	test_driver_data->size = 0;
+	test_driver_data->api = 0;
+	test_driver_data->written = false;
+}
+
+static int test_load_driver_data(struct driver_data_test_device *test_dev,
+				 const struct firmware *driver_data)
+{
+	struct test_driver_data_private *test_driver_data =
+		&test_dev->test_driver_data;
+	int ret = 0;
+
+	if (!driver_data)
+		return -ENOENT;
+
+	mutex_lock(&test_dev->driver_data_mutex);
+
+	free_test_driver_data(test_driver_data);
+
+	test_driver_data->data = kzalloc(driver_data->size, GFP_KERNEL);
+	if (!test_driver_data->data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(test_driver_data->data, driver_data->data, driver_data->size);
+	test_driver_data->size = driver_data->size;
+	test_driver_data->written = true;
+	test_driver_data->api = driver_data->api;
+
+	dev_info(test_dev->dev, "loaded: %zu\n", test_driver_data->size);
+
+out:
+	mutex_unlock(&test_dev->driver_data_mutex);
+
+	return ret;
+}
+
+static int sync_found_cb(void *context, const struct firmware *driver_data)
+{
+	struct driver_data_test_device *test_dev = context;
+	int ret;
+
+	ret = test_load_driver_data(test_dev, driver_data);
+	if (ret)
+		dev_info(test_dev->dev,
+			 "unable to write driver_data: %d\n", ret);
+	return ret;
+}
+
+static ssize_t config_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int len = 0;
+
+	mutex_lock(&test_dev->config_mutex);
+
+	len += snprintf(buf, PAGE_SIZE,
+			"Custom trigger configuration for: %s\n",
+			dev_name(dev));
+
+	if (config->default_name)
+		len += snprintf(buf+len, PAGE_SIZE,
+				"default name:\t%s\n",
+				config->default_name);
+	else
+		len += snprintf(buf+len, PAGE_SIZE,
+				"default name:\tEMTPY\n");
+
+	if (config->name)
+		len += snprintf(buf+len, PAGE_SIZE,
+				"name:\t\t%s\n", config->name);
+	else
+		len += snprintf(buf+len, PAGE_SIZE,
+				"name:\t\tEMPTY\n");
+
+	len += snprintf(buf+len, PAGE_SIZE,
+			"type:\t\t%s\n",
+			config->async ? "async" : "sync");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"optional:\t%s\n",
+			config->optional ? "true" : "false");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"enable_opt_cb:\t%s\n",
+			config->enable_opt_cb ? "true" : "false");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"use_api_versioning:\t%s\n",
+			config->use_api_versioning ? "true" : "false");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"api_min:\t%u\n", config->api_min);
+	len += snprintf(buf+len, PAGE_SIZE,
+			"api_max:\t%u\n", config->api_max);
+	if (config->api_name_postfix)
+		len += snprintf(buf+len, PAGE_SIZE,
+				"api_name_postfix:\t\t%s\n", config->api_name_postfix);
+	else
+		len += snprintf(buf+len, PAGE_SIZE,
+				"api_name_postfix:\t\tEMPTY\n");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"keep:\t\t%s\n",
+			config->keep ? "true" : "false");
+
+	mutex_unlock(&test_dev->config_mutex);
+
+	return len;
+}
+static DEVICE_ATTR_RO(config);
+
+static int config_load_data(struct driver_data_test_device *test_dev,
+			    const struct firmware *driver_data)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	ret = test_load_driver_data(test_dev, driver_data);
+	if (ret) {
+		if (!config->optional)
+			dev_info(test_dev->dev,
+				 "unable to write driver_data\n");
+	}
+	if (config->keep) {
+		release_firmware(driver_data);
+		driver_data = NULL;
+	}
+
+	return ret;
+}
+
+static int config_req_default(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+	/*
+	 * Note: we don't chain config->optional here, we make this
+	 * fallback file a requirement. It doesn't make much sense to test
+	 * chaining further as the optional callback is implementation
+	 * specific, by testing it once we test it for any possible
+	 * chains. We provide this as an example of what people can do
+	 * and use a default non-optional fallback.
+	 */
+	const struct driver_data_req_params req_params = {
+		DRIVER_DATA_DEFAULT_SYNC(sync_found_cb, test_dev),
+	};
+
+	if (config->async)
+		dev_info(test_dev->dev,
+			 "loading default fallback '%s' using sync request now\n",
+			 config->default_name);
+	else
+		dev_info(test_dev->dev,
+			 "loading default fallback '%s'\n",
+			 config->default_name);
+
+	ret = driver_data_request_sync(config->default_name,
+				       &req_params, test_dev->dev);
+	if (ret)
+		dev_info(test_dev->dev,
+			 "load of default '%s' failed: %d\n",
+			 config->default_name, ret);
+
+	return ret;
+}
+
+/*
+ * This is the default sync fallback callback, as a fallback this
+ * then uses a sync request.
+ */
+static int config_sync_req_default_cb(void *context)
+{
+	struct driver_data_test_device *test_dev = context;
+	int ret;
+
+	ret = config_req_default(test_dev);
+
+	return ret;
+
+	/* Leave all the error checking for the main caller */
+}
+
+/*
+ * This is the default config->async fallback callback, as a fallback this
+ * then uses a sync request.
+ */
+static void config_async_req_default_cb(void *context)
+{
+	struct driver_data_test_device *test_dev = context;
+
+	config_req_default(test_dev);
+
+	complete(&test_dev->request_complete);
+	/* Leave all the error checking for the main caller */
+}
+
+static int config_sync_req_cb(void *context,
+			      const struct firmware *driver_data)
+{
+	struct driver_data_test_device *test_dev = context;
+
+	return config_load_data(test_dev, driver_data);
+}
+
+static int trigger_config_sync(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+	const struct driver_data_req_params req_params_default = {
+		DRIVER_DATA_DEFAULT_SYNC(config_sync_req_cb, test_dev),
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct driver_data_req_params req_params_opt_cb = {
+		DRIVER_DATA_DEFAULT_SYNC(config_sync_req_cb, test_dev),
+		DRIVER_DATA_SYNC_OPT_CB(config_sync_req_default_cb, test_dev),
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct driver_data_req_params *req_params;
+
+	if (config->enable_opt_cb)
+		req_params = &req_params_opt_cb;
+	else
+		req_params = &req_params_default;
+
+	ret = driver_data_request_sync(config->name, req_params, test_dev->dev);
+	if (ret)
+		dev_err(test_dev->dev, "sync load of '%s' failed: %d\n",
+			config->name, ret);
+
+	return ret;
+}
+
+static void config_async_req_cb(const struct firmware *driver_data,
+				void *context)
+{
+	struct driver_data_test_device *test_dev = context;
+
+	config_load_data(test_dev, driver_data);
+	complete(&test_dev->request_complete);
+}
+
+static int config_async_req_api_cb(const struct firmware *driver_data,
+				   void *context)
+{
+	struct driver_data_test_device *test_dev = context;
+	/*
+	 * This drivers may process a file and determine it does not
+	 * like it, so it wants us to try again, to do this it returns
+	 * -EAGAIN. We mimick this behaviour by not liking odd numbered
+	 * api files, so we know to expect only success on even numbered
+	 * apis.
+	 */
+	if (driver_data && (driver_data->api % 2 == 1)) {
+		pr_info("File api %u found but we purposely ignore it\n",
+			driver_data->api);
+		return -EAGAIN;
+	}
+
+	config_load_data(test_dev, driver_data);
+
+	/*
+	 * If the file was found we let our stupid driver emulator thing
+	 * fake holding the driver data. If the file was not found just
+	 * bail immediately.
+	 */
+	if (driver_data)
+		pr_info("File with api %u found!\n", driver_data->api);
+
+	complete(&test_dev->request_complete);
+
+	return 0;
+}
+
+static int trigger_config_async(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+	const struct driver_data_req_params req_params_default = {
+		DRIVER_DATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
+		.sync_reqs.mode = config->async ?
+			DRIVER_DATA_ASYNC : DRIVER_DATA_SYNC,
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct driver_data_req_params req_params_opt_cb = {
+		DRIVER_DATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
+		DRIVER_DATA_ASYNC_OPT_CB(config_async_req_default_cb, test_dev),
+		.sync_reqs.mode = config->async ?
+			DRIVER_DATA_ASYNC : DRIVER_DATA_SYNC,
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct driver_data_req_params req_params_api = {
+		DRIVER_DATA_API_CB(config_async_req_api_cb, test_dev),
+		.sync_reqs.mode = config->async ?
+			DRIVER_DATA_ASYNC : DRIVER_DATA_SYNC,
+		.optional = config->optional,
+		.keep = config->keep,
+		DRIVER_DATA_API(config->api_min, config->api_max, config->api_name_postfix),
+		.uses_api_versioning = config->use_api_versioning,
+	};
+	const struct driver_data_req_params *req_params;
+
+	if (config->enable_opt_cb)
+		req_params = &req_params_opt_cb;
+	else if (config->use_api_versioning)
+		req_params = &req_params_api;
+	else
+		req_params = &req_params_default;
+
+	test_dev->api_found_calls = 0;
+	ret = driver_data_request_async(config->name, req_params,
+					test_dev->dev);
+	if (ret) {
+		dev_err(test_dev->dev, "async load of '%s' failed: %d\n",
+			config->name, ret);
+		goto out;
+	}
+
+	/*
+	 * Without waiting for completion we'd return before the async callback
+	 * completes, and any testing analysis done on the results would be
+	 * bogus. We could have used async cookies to avoid having drivers
+	 * avoid adding their own completions and initializing them.
+	 * We have decided its best to keep with the old way of doing things to
+	 * keep things compatible. Deal with it.
+	 */
+	wait_for_completion_timeout(&test_dev->request_complete, 5 * HZ);
+
+out:
+	return ret;
+}
+
+static ssize_t
+trigger_config_store(struct device *dev,
+		     struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_driver_data_private *test_driver_data =
+		&test_dev->test_driver_data;
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->trigger_mutex);
+	mutex_lock(&test_dev->config_mutex);
+
+	dev_info(dev, "loading '%s'\n", config->name);
+
+	if (config->async)
+		ret = trigger_config_async(test_dev);
+	else
+		ret = trigger_config_sync(test_dev);
+
+	config->test_result = ret;
+
+	if (ret)
+		goto out;
+
+	if (test_driver_data->written) {
+		dev_info(dev, "loaded: %zu\n", test_driver_data->size);
+		ret = count;
+	} else {
+		dev_err(dev, "failed to load firmware\n");
+		ret = -ENODEV;
+	}
+
+out:
+	mutex_unlock(&test_dev->config_mutex);
+	mutex_unlock(&test_dev->trigger_mutex);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(trigger_config);
+
+/*
+ * XXX: move to kstrncpy() once merged.
+ *
+ * Users should use kfree_const() when freeing these.
+ */
+static int __kstrncpy(char **dst, const char *name, size_t count, gfp_t gfp)
+{
+	*dst = kstrndup(name, count, gfp);
+	if (!*dst)
+		return -ENOSPC;
+	return count;
+}
+
+static void __driver_data_config_free(struct test_config *config)
+{
+	kfree_const(config->name);
+	config->name = NULL;
+	kfree_const(config->default_name);
+	config->default_name = NULL;
+	kfree_const(config->api_name_postfix);
+	config->api_name_postfix = NULL;
+}
+
+static void driver_data_config_free(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	mutex_lock(&test_dev->config_mutex);
+	__driver_data_config_free(config);
+	mutex_unlock(&test_dev->config_mutex);
+}
+
+static int __driver_data_config_init(struct test_config *config)
+{
+	int ret;
+
+	ret = __kstrncpy(&config->name, TEST_DRIVER_DATA,
+			 strlen(TEST_DRIVER_DATA), GFP_KERNEL);
+	if (ret < 0)
+		goto out;
+
+	ret = __kstrncpy(&config->default_name, TEST_DRIVER_DATA,
+			 strlen(TEST_DRIVER_DATA), GFP_KERNEL);
+	if (ret < 0)
+		goto out;
+
+	config->async = false;
+	config->optional = false;
+	config->keep = false;
+	config->enable_opt_cb = false;
+	config->use_api_versioning = false;
+	config->api_min = 0;
+	config->api_max = 0;
+	config->test_result = 0;
+
+	return 0;
+
+out:
+	__driver_data_config_free(config);
+	return ret;
+}
+
+int driver_data_config_init(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	ret = __driver_data_config_init(config);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t config_name_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	kfree_const(config->name);
+	ret = __kstrncpy(&config->name, buf, count, GFP_KERNEL);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+/*
+ * As per sysfs_kf_seq_show() the buf is max PAGE_SIZE.
+ */
+static ssize_t config_test_show_str(struct mutex *config_mutex,
+				    char *dst,
+				    char *src)
+{
+	int len;
+
+	mutex_lock(config_mutex);
+	len = snprintf(dst, PAGE_SIZE, "%s\n", src);
+	mutex_unlock(config_mutex);
+
+	return len;
+}
+
+static ssize_t config_name_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return config_test_show_str(&test_dev->config_mutex, buf,
+				    config->name);
+}
+static DEVICE_ATTR(config_name, 0644, config_name_show, config_name_store);
+
+static ssize_t config_default_name_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	kfree_const(config->default_name);
+	ret = __kstrncpy(&config->default_name, buf, count, GFP_KERNEL);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t config_default_name_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return config_test_show_str(&test_dev->config_mutex, buf,
+				    config->default_name);
+}
+static DEVICE_ATTR(config_default_name, 0644, config_default_name_show,
+		   config_default_name_store);
+
+static ssize_t reset_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->trigger_mutex);
+
+	mutex_lock(&test_dev->driver_data_mutex);
+	free_test_driver_data(&test_dev->test_driver_data);
+	reinit_completion(&test_dev->request_complete);
+	mutex_unlock(&test_dev->driver_data_mutex);
+
+	mutex_lock(&test_dev->config_mutex);
+
+	__driver_data_config_free(config);
+
+	ret = __driver_data_config_init(config);
+	if (ret < 0) {
+		ret = -ENOMEM;
+		dev_err(dev, "could not alloc settings for config trigger: %d\n",
+		       ret);
+		goto out;
+	}
+
+	dev_info(dev, "reset\n");
+	ret = count;
+
+out:
+	mutex_unlock(&test_dev->config_mutex);
+	mutex_unlock(&test_dev->trigger_mutex);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(reset);
+
+/*
+ * XXX: consider a soluton to generalize drivers to specify their own
+ * mutex, adding it to dev core after this gets merged. This may not
+ * be important for once-in-a-while system tuning parameters, but if
+ * we want to enable fuzz testing, this is really important.
+ *
+ * It may make sense to just have a "struct device configuration mutex"
+ * for these sorts of things, although there is difficulty in that we'd
+ * need dynamically allocated attributes for that. Its the same reason
+ * why we ended up not using the provided standard device attribute
+ * bool, int interfaces.
+ */
+
+static int test_dev_config_update_bool(struct driver_data_test_device *test_dev,
+				       const char *buf, size_t size,
+				       bool *config)
+{
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	if (strtobool(buf, config) < 0)
+		ret = -EINVAL;
+	else
+		ret = size;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t
+test_dev_config_show_bool(struct driver_data_test_device *test_dev,
+			  char *buf,
+			  bool config)
+{
+	bool val;
+
+	mutex_lock(&test_dev->config_mutex);
+	val = config;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static int test_dev_config_update_int(struct driver_data_test_device *test_dev,
+				      const char *buf, size_t size,
+				      int *config)
+{
+	int ret;
+	long new;
+
+	ret = kstrtol(buf, 10, &new);
+	if (ret)
+		return ret;
+
+	if (new > INT_MAX || new < INT_MIN)
+		return -EINVAL;
+
+	mutex_lock(&test_dev->config_mutex);
+	*(int *)config = new;
+	mutex_unlock(&test_dev->config_mutex);
+
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
+static
+ssize_t test_dev_config_show_int(struct driver_data_test_device *test_dev,
+				 char *buf,
+				 int config)
+{
+	int val;
+
+	mutex_lock(&test_dev->config_mutex);
+	val = config;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static int test_dev_config_update_u8(struct driver_data_test_device *test_dev,
+				     const char *buf, size_t size,
+				     u8 *config)
+{
+	int ret;
+	long new;
+
+	ret = kstrtol(buf, 10, &new);
+	if (ret)
+		return ret;
+
+	if (new > U8_MAX)
+		return -EINVAL;
+
+	mutex_lock(&test_dev->config_mutex);
+	*(u8 *)config = new;
+	mutex_unlock(&test_dev->config_mutex);
+
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
+static
+ssize_t test_dev_config_show_u8(struct driver_data_test_device *test_dev,
+				char *buf,
+				u8 config)
+{
+	u8 val;
+
+	mutex_lock(&test_dev->config_mutex);
+	val = config;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+
+static ssize_t config_async_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->async);
+}
+
+static ssize_t config_async_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf, config->async);
+}
+static DEVICE_ATTR(config_async, 0644, config_async_show, config_async_store);
+
+static ssize_t config_optional_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->optional);
+}
+
+static ssize_t config_optional_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf, config->optional);
+}
+static DEVICE_ATTR(config_optional, 0644, config_optional_show,
+		   config_optional_store);
+
+static ssize_t config_keep_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->keep);
+}
+
+static ssize_t config_keep_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf, config->keep);
+}
+static DEVICE_ATTR(config_keep, 0644, config_keep_show, config_keep_store);
+
+static ssize_t config_enable_opt_cb_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->enable_opt_cb);
+}
+
+static ssize_t config_enable_opt_cb_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf,
+					 config->enable_opt_cb);
+}
+static DEVICE_ATTR(config_enable_opt_cb, 0644,
+		   config_enable_opt_cb_show,
+		   config_enable_opt_cb_store);
+
+static ssize_t config_use_api_versioning_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->use_api_versioning);
+}
+
+static ssize_t config_use_api_versioning_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf,
+					 config->use_api_versioning);
+}
+static DEVICE_ATTR(config_use_api_versioning, 0644,
+		   config_use_api_versioning_show,
+		   config_use_api_versioning_store);
+
+static ssize_t config_api_min_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_u8(test_dev, buf, count,
+					 &config->api_min);
+}
+
+static ssize_t config_api_min_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_u8(test_dev, buf, config->api_min);
+}
+static DEVICE_ATTR(config_api_min, 0644, config_api_min_show, config_api_min_store);
+
+static ssize_t config_api_max_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_u8(test_dev, buf, count,
+					 &config->api_max);
+}
+
+static ssize_t config_api_max_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_u8(test_dev, buf, config->api_max);
+}
+static DEVICE_ATTR(config_api_max, 0644, config_api_max_show, config_api_max_store);
+
+static ssize_t config_api_name_postfix_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	kfree_const(config->api_name_postfix);
+	ret = __kstrncpy(&config->api_name_postfix, buf, count, GFP_KERNEL);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t config_api_name_postfix_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return config_test_show_str(&test_dev->config_mutex, buf,
+				    config->api_name_postfix);
+}
+static DEVICE_ATTR(config_api_name_postfix, 0644, config_api_name_postfix_show,
+		   config_api_name_postfix_store);
+
+static ssize_t test_result_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_int(test_dev, buf, count,
+					  &config->test_result);
+}
+
+static ssize_t test_result_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_int(test_dev, buf, config->test_result);
+}
+static DEVICE_ATTR(test_result, 0644, test_result_show, test_result_store);
+
+#define TEST_DRIVER_DATA_DEV_ATTR(name)		&dev_attr_##name.attr
+
+static struct attribute *test_dev_attrs[] = {
+	TEST_DRIVER_DATA_DEV_ATTR(trigger_config),
+	TEST_DRIVER_DATA_DEV_ATTR(config),
+	TEST_DRIVER_DATA_DEV_ATTR(reset),
+
+	TEST_DRIVER_DATA_DEV_ATTR(config_name),
+	TEST_DRIVER_DATA_DEV_ATTR(config_default_name),
+	TEST_DRIVER_DATA_DEV_ATTR(config_async),
+	TEST_DRIVER_DATA_DEV_ATTR(config_optional),
+	TEST_DRIVER_DATA_DEV_ATTR(config_keep),
+	TEST_DRIVER_DATA_DEV_ATTR(config_use_api_versioning),
+	TEST_DRIVER_DATA_DEV_ATTR(config_enable_opt_cb),
+	TEST_DRIVER_DATA_DEV_ATTR(config_api_min),
+	TEST_DRIVER_DATA_DEV_ATTR(config_api_max),
+	TEST_DRIVER_DATA_DEV_ATTR(config_api_name_postfix),
+	TEST_DRIVER_DATA_DEV_ATTR(test_result),
+
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(test_dev);
+
+void free_test_dev_driver_data(struct driver_data_test_device *test_dev)
+{
+	kfree_const(test_dev->misc_dev.name);
+	test_dev->misc_dev.name = NULL;
+	vfree(test_dev);
+	test_dev = NULL;
+	driver_data_config_free(test_dev);
+}
+
+void unregister_test_dev_driver_data(struct driver_data_test_device *test_dev)
+{
+	wait_for_completion_timeout(&test_dev->request_complete, 5 * HZ);
+	dev_info(test_dev->dev, "removing interface\n");
+	misc_deregister(&test_dev->misc_dev);
+	kfree(&test_dev->misc_dev.name);
+	free_test_dev_driver_data(test_dev);
+}
+
+struct driver_data_test_device *alloc_test_dev_driver_data(int idx)
+{
+	int ret;
+	struct driver_data_test_device *test_dev;
+	struct miscdevice *misc_dev;
+
+	test_dev = vzalloc(sizeof(struct driver_data_test_device));
+	if (!test_dev)
+		goto err_out;
+
+	mutex_init(&test_dev->driver_data_mutex);
+	mutex_init(&test_dev->config_mutex);
+	mutex_init(&test_dev->trigger_mutex);
+	init_completion(&test_dev->request_complete);
+
+	ret = driver_data_config_init(test_dev);
+	if (ret < 0)
+		goto err_out_free;
+
+	test_dev->dev_idx = idx;
+	misc_dev = &test_dev->misc_dev;
+
+	misc_dev->minor = MISC_DYNAMIC_MINOR;
+	misc_dev->name = kasprintf(GFP_KERNEL, "test_driver_data%d", idx);
+	if (!misc_dev->name)
+		goto err_out_free_config;
+
+	misc_dev->fops = &test_fw_fops;
+	misc_dev->groups = test_dev_groups;
+
+	return test_dev;
+
+err_out_free_config:
+	__driver_data_config_free(&test_dev->config);
+err_out_free:
+	kfree(test_dev);
+err_out:
+	return NULL;
+}
+
+static int register_test_dev_driver_data(void)
+{
+	struct driver_data_test_device *test_dev = NULL;
+	int ret = -ENODEV;
+
+	mutex_lock(&reg_dev_mutex);
+
+	/* int should suffice for number of devices, test for wrap */
+	if (unlikely(num_test_devs + 1) < 0) {
+		pr_err("reached limit of number of test devices\n");
+		goto out;
+	}
+
+	test_dev = alloc_test_dev_driver_data(num_test_devs);
+	if (!test_dev) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = misc_register(&test_dev->misc_dev);
+	if (ret) {
+		pr_err("could not register misc device: %d\n", ret);
+		free_test_dev_driver_data(test_dev);
+		goto out;
+	}
+
+	test_dev->dev = test_dev->misc_dev.this_device;
+	list_add_tail(&test_dev->list, &reg_test_devs);
+	dev_info(test_dev->dev, "interface ready\n");
+
+	num_test_devs++;
+
+out:
+	mutex_unlock(&reg_dev_mutex);
+
+	return ret;
+}
+
+static int __init test_driver_data_init(void)
+{
+	int ret;
+
+	ret = register_test_dev_driver_data();
+	if (ret)
+		pr_err("Cannot add first test driver_data device\n");
+
+	return ret;
+}
+late_initcall(test_driver_data_init);
+
+static void __exit test_driver_data_exit(void)
+{
+	struct driver_data_test_device *test_dev, *tmp;
+
+	mutex_lock(&reg_dev_mutex);
+	list_for_each_entry_safe(test_dev, tmp, &reg_test_devs, list) {
+		list_del(&test_dev->list);
+		unregister_test_dev_driver_data(test_dev);
+	}
+	mutex_unlock(&reg_dev_mutex);
+}
+
+module_exit(test_driver_data_exit);
+
+MODULE_AUTHOR("Luis R. Rodriguez <[email protected]>");
+MODULE_LICENSE("GPL");

tools/testing/selftests/firmware/Makefile +1 -1

index 1894d625af2d..c9bf6c44435f 100644
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_filesystem.sh fw_fallback.sh driver_data.sh
 
 include ../lib.mk
 

tools/testing/selftests/firmware/config +1 -0

index c8137f70e291..0f1a299f9270 100644
@@ -1 +1,2 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_TEST_DRIVER_DATA=y

tools/testing/selftests/firmware/driver_data.sh new file +33 -0

new file mode 100755
index 000000000000..085fbaec6b3e
@@ -0,0 +1,996 @@
+#!/bin/bash
+# Copyright (C) 2016 Luis R. Rodriguez <[email protected]>
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of copyleft-next (version 0.3.1 or later) as published
+# at http://copyleft-next.org/.
+
+# This performs a series tests against firmware_class to excercise the
+# firmware_class driver with focus only on the extensible driver data API.
+#
+# To make this test self contained, and not pollute your distribution
+# firmware install paths, we reset the custom load directory to a
+# temporary location.
+
+set -e
+
+TEST_NAME="driver_data"
+TEST_DRIVER="test_${TEST_NAME}"
+TEST_DIR=$(dirname $0)
+
+# This represents
+#
+# TEST_ID:TEST_COUNT:ENABLED
+#
+# TEST_ID: is the test id number
+# TEST_COUNT: number of times we should run the test
+# ENABLED: 1 if enabled, 0 otherwise
+#
+# Once these are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+ALL_TESTS="0001:3:1"
+ALL_TESTS="$ALL_TESTS 0002:3:1"
+ALL_TESTS="$ALL_TESTS 0003:3:1"