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

["takahiro.akashi@linaro.org" <takahiro.akashi@linaro.org>]

On Mon, Apr 10, 2017 at 12:42:44PM +0000, Coelho, Luciano wrote:
> 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@kernel.org>
> > ---
> >  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..."

Or, Users of ... have grown in number, including ...
> 
> > +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*.

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.

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

driver_data_request_sync

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

req_params

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

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)

Thanks,
-Takahiro AKASHI

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