Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753999AbdDKH6k (ORCPT ); Tue, 11 Apr 2017 03:58:40 -0400 Received: from mail-pg0-f41.google.com ([74.125.83.41]:35897 "EHLO mail-pg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751799AbdDKH6i (ORCPT ); Tue, 11 Apr 2017 03:58:38 -0400 Date: Tue, 11 Apr 2017 17:01:51 +0900 From: "takahiro.akashi@linaro.org" To: "Coelho, Luciano" Cc: "gregkh@linuxfoundation.org" , "mcgrof@kernel.org" , "linux-kernel@vger.kernel.org" , "pjones@redhat.com" , "moritz.fischer@ettus.com" , "dhowells@redhat.com" , "pmladek@suse.com" , "Berg, Johannes" , "rjw@rjwysocki.net" , "yi1.li@linux.intel.com" , "kvalo@codeaurora.org" , "luto@kernel.org" , "arend.vanspriel@broadcom.com" , "rafal@milecki.pl" , "dwmw2@infradead.org" , "wagi@monom.org" , "atull@opensource.altera.com" , "Grumbach, Emmanuel" Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API Message-ID: <20170411080149.GD15139@linaro.org> Mail-Followup-To: "takahiro.akashi@linaro.org" , "Coelho, Luciano" , "gregkh@linuxfoundation.org" , "mcgrof@kernel.org" , "linux-kernel@vger.kernel.org" , "pjones@redhat.com" , "moritz.fischer@ettus.com" , "dhowells@redhat.com" , "pmladek@suse.com" , "Berg, Johannes" , "rjw@rjwysocki.net" , "yi1.li@linux.intel.com" , "kvalo@codeaurora.org" , "luto@kernel.org" , "arend.vanspriel@broadcom.com" , "rafal@milecki.pl" , "dwmw2@infradead.org" , "wagi@monom.org" , "atull@opensource.altera.com" , "Grumbach, Emmanuel" References: <20170330032514.17173-1-mcgrof@kernel.org> <20170330032514.17173-3-mcgrof@kernel.org> <1491828162.31980.50.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1491828162.31980.50.camel@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13637 Lines: 335 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 > > --- > > 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 = ¶ms->priv_params; > > + const struct driver_data_req_params *req_params = ¶ms->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(¶ms->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.