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

[Coelho, Luciano]

On Wed, 2017-03-29 at 20:25 -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 +
>  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

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


> +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.


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

"only" seems irrelevant here.


> +
> +  * 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*.


> +
> +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.

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

[...]

> 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

[...]

> @@ -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;

Braces.


> +	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.

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


>  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;

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.


> +/**
> + * 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;

Same here.

--
Luca.