Message-ID: <20170427031625.GI28800@wotan.suse.de>
Date:   Thu, 27 Apr 2017 05:16:25 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     "Coelho, Luciano" <luciano.coelho@...el.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pjones@...hat.com" <pjones@...hat.com>,
        "moritz.fischer@...us.com" <moritz.fischer@...us.com>,
        "takahiro.akashi@...aro.org" <takahiro.akashi@...aro.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "Berg, Johannes" <johannes.berg@...el.com>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "yi1.li@...ux.intel.com" <yi1.li@...ux.intel.com>,
        "kvalo@...eaurora.org" <kvalo@...eaurora.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "arend.vanspriel@...adcom.com" <arend.vanspriel@...adcom.com>,
        "rafal@...ecki.pl" <rafal@...ecki.pl>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "wagi@...om.org" <wagi@...om.org>,
        "atull@...nsource.altera.com" <atull@...nsource.altera.com>,
        "Grumbach, Emmanuel" <emmanuel.grumbach@...el.com>
Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API

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@...nel.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..."

Rephrased as :

"The firmware API has grown to include users which are not looking for" ...

Well actually dropped due to the below feedback as well.

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

Alright, trimmed:

+The driver data APIs provides a flexible API for general driver data file
+lookups. Its flexibility aims at mitigating 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:
> 
> "only" seems irrelevant here.

Removed.

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

True, however its important to clarify a bit what is mean by a flexible API, so
replaced the last sentence with:

""
The flexibility of the API is provided by expanding the request parameters as
new functionality is needed, without loosely modifying or adding new exported
APIs.  
""

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

Makes sense, just moved it to the wiki.

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

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

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

Sorry, the @desc was the old name, due to bike-shedding its now @params, I've
updated this and rephrased it a bit better:

""
Callers get access to any found driver data meeting the specified criteria
through an optional callback set on @params. 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.
""

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

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.

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

  Luis

Powered by blists - more mailing lists