Re: [PATCH v6 2/5] firmware: add extensible driver data API

[AKASHI Takahiro]

On Wed, Mar 29, 2017 at 08:25:11PM -0700, Luis R. Rodriguez wrote:
> 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 <mcgrof@xxxxxxxxxx>
> ---
>  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.

>  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
> 
> diff --git a/Documentation/driver-api/firmware/driver_data.rst b/Documentation/driver-api/firmware/driver_data.rst
> new file mode 100644
> index 000000000000..08407b7568fe
> --- /dev/null
> +++ b/Documentation/driver-api/firmware/driver_data.rst
> @@ -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
> diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
> index 1abe01793031..c2be92e2628c 100644
> --- a/Documentation/driver-api/firmware/index.rst
> +++ b/Documentation/driver-api/firmware/index.rst
> @@ -7,6 +7,7 @@ Linux Firmware API
>     introduction
>     core
>     request_firmware
> +   driver_data
>
>  .. only::  subproject and html
>  
> diff --git a/Documentation/driver-api/firmware/introduction.rst b/Documentation/driver-api/firmware/introduction.rst
> index 211cb44eb972..a0f6a3fa1d5d 100644
> --- a/Documentation/driver-api/firmware/introduction.rst
> +++ b/Documentation/driver-api/firmware/introduction.rst
> @@ -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
>  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

> +
> +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.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 939311d601b8..3f025f738600 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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 <mcgrof@xxxxxxxxxx>
>  L:	linux-kernel@xxxxxxxxxxxxxxx
>  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 <josh.h.morris@xxxxxxxxxx>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index f702566554e1..cc3c2247980c 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -2,6 +2,7 @@
>   * firmware_class.c - Multi purpose firmware loading support
>   *
>   * Copyright (c) 2003 Manuel Estrada Sainz
> + * Copyright (c) 2017 Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
>   *
>   * 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;
> +
> +	sync_reqs = &dfl_sync_reqs;
> +
> +	__module_get(sync_reqs->module);
> +	get_device(device);
> +
> +	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.

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.

> +	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);
>  
> diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
> index c3d3a4129838..fda1e716017d 100644
> --- a/include/linux/driver_data.h
> +++ b/include/linux/driver_data.h
> @@ -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
> @@ -59,20 +100,128 @@ struct driver_data_reqs {
>   */
>  struct driver_data_req_params {
>  	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.)

Thanks,
-Takahiro AKASHI

> +	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 */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0ccb8ac..3a71924d35d7 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -13,6 +13,8 @@ struct firmware {
>  	const u8 *data;
>  	struct page **pages;
>  
> +	u8 api;
> +
>  	/* firmware loader private fields */
>  	void *priv;
>  };
> -- 
> 2.11.0
>