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

takahiro.akashi
2017-04-11T07:58:40Z

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.