Message-ID: <20170119113652.GP28024@kroah.com>
Date:   Thu, 19 Jan 2017 12:36:52 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     ming.lei@...onical.com, bp@...en8.de, wagi@...om.org, teg@...m.no,
        mchehab@....samsung.com, zajec5@...il.com,
        linux-kernel@...r.kernel.org, markivx@...eaurora.org,
        stephen.boyd@...aro.org, broonie@...nel.org,
        zohar@...ux.vnet.ibm.com, tiwai@...e.de, johannes@...solutions.net,
        chunkeey@...glemail.com, hauke@...ke-m.de,
        jwboyer@...oraproject.org, dmitry.torokhov@...il.com,
        dwmw2@...radead.org, jslaby@...e.com,
        torvalds@...ux-foundation.org, luto@...capital.net,
        fengguang.wu@...el.com, rpurdie@...ys.net,
        j.anaszewski@...sung.com, Abhay_Salunke@...l.com,
        Julia.Lawall@...6.fr, Gilles.Muller@...6.fr, nicolas.palix@...g.fr,
        dhowells@...hat.com, bjorn.andersson@...aro.org,
        arend.vanspriel@...adcom.com, kvalo@...eaurora.org
Subject: Re: [PATCH v4 1/3] firmware: add new extensible firmware API -
 drvdata

On Thu, Jan 12, 2017 at 07:02:42AM -0800, Luis R. Rodriguez wrote:
> The firmware API has evolved over the years slowly, as it
> grows we extend it by adding new routines or at times we extend
> existing routines with more or less arguments. This doesn't scale
> well, when new arguments are added to existing routines it 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", an example here is the p54 driver
> enables users to provide a custom EEPROM through this interface.
> Another example is optional CPU microcode updates. This list is
> actually quite endless...
> 
> There are other subsystems which would like to make use of the
> 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. If different requirements are needed it would
> again mean adding more arguments and making a slew of collateral
> evolutions, or adding yet-another-new-API-call (TM).
> 
> Another sticking point over the current firmware API is that
> some callers may need the firmware fallback mechanism when its
> enabled. There are two types of fallback mechanisms and both have
> shortcomings. This new API accepts the current status quo and
> ignore the fallback mechanism all together. When and if we add
> support for it, it will be well though out.
> 
> This new extensible firmware API enables new extensions to be added by
> avoiding future unnecessary collateral evolutions as this code /
> features get added. This new set of APIs leaves the old firmware API
> as-is, ignores all firmware fallback mechanism, labels the new
> API to reflect its broad use outside of the scope of firmware: driver
> data helpers, and builds on top of the original firmware core code.
> We purposely try to limit the scope of changes in this new API to
> simply enable a flexible API to start off with.
> 
> The new extensible "driver data" set of helpers accepts that there
> really are only two types of requests for accessing driver data:
> 
> a) synchronous requests
> b) asynchronous requests
> 
> Both of these requests may have a different set of requirements which
> must be met. These requirements can simply be passed as a struct
> drvdata_req_params to each type of request. This struct can be extended
> over time to support different requirements as the kernel evolves.
> 
> Using the new driver data helpers is only necessary if you have
> requirements outside of what the existing old firmware API accepts
> or alternatively if you want to ensure to avoid the old firmware
> fallback mechanism at all times, regardless of what kernel your driver
> might run in.
> 
> Developers with new uses should extend the new new struct drvdata_req_params
> and driver data code to provide support for new features.
> 
> A *few* simple features added as part of the new set of driver data
> request APIs, other than making the new API easily extensible for
> the future:
> 
>  - The firmware fallback mechanism is currenlty always ignored
>  - 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 descriptor. The new
>    drvdata API is able to free the drvdata file for you by requiring a
>    consumer callback for the driver data file.
>  - You no longer need to declare and use your own completions, you
>    can replace your completions with drvdata_synchronize_request() using
>    the async_cookie set for you by drvdata_file_request_async(). When
>    drvdata_file_request_async() completes you can rest assured all the
>    work for both triggering, and processing the drvdata using any of
>    your callbacks has completed.
>  - Allow 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 -- although it should be
>    noted another one of its goal was to also skip the fallback mechanisms.
>    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.
>  - The driver data request APIs currently match the old synchronous firmware
>    API calls to refcounted firmware_class module, but it should be easy
>    to add support now to enable also refcounting the caller's module
>    should it be be needed. Likewise the driver data request APIs match the
>    old asynchronous firmware API call and refcounts the caller's module.

I think this changelog novel is longer than the documentation you added
to the kernel :(

> --- /dev/null
> +++ b/Documentation/driver-api/firmware/drvdata.rst
> @@ -0,0 +1,91 @@
> +===========
> +drvdata API

Here kid, have a few vowels, we have plenty...

Please spell this out "driver_data", there's no need to shorten it for
no reason at all except to confuse people / non-native speakers for a
while before they figure it out.

> +===========
> +
> +As the kernel evolves we keep extending the firmware_class set of APIs
> +with more or less arguments, this creates a slew of collateral evolutions.

Why is this sentance here?

> +The set of users of firmware request APIs has also grown now to include
> +users which are not looking for "firmware" per se, but instead general
> +driver data files which for one reason or another has been decided to be
> +kept oustide of the kernel, and/or to allow dynamic updates. The driver data
> +request set of APIs addresses rebranding of firmware as generic driver data
> +files, and provides a way to enable these APIs to easily be extended without
> +much collateral evolutions.
> +
> +Driver data modes of operation
> +==============================
> +
> +There are only two types of modes of operation for system data requests:
> +
> +  * synchronous  - drvdata_request()
> +  * asynchronous - drvdata_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 can grow as with new
> +fields as requirements grow. The old firmware API provides two synchronous
> +requests: request_firmware() and request_firmware_direct(), the later allowing
> +the caller to specify that the "driver data file" is optional.  The driver data
> +request API allows a caller to set the optional nature of the driver data
> +on the request parameter data structure using the same synchronous API. Since
> +this requirement is part of the request paramter data structure it also allows
> +asynchronous requests to specify that the driver data is optional.
> +
> +Reference counting and releasing the system 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 mimic this behaviour, for synchronous
> +requests the firmware_class module is refcounted through the use of
> +dfl_sync_reqs, although if in the future we may later enable use of
> +also refcounting the caller's module as well. Likewise in the future we
> +may extend asynchronous calls to refcount the firmware_class module.
> +
> +Typical use of the old synchronous firmware APIs consist of the caller
> +requesting for "driver data", consuming it after a request and finally
> +freeing it. Typical asynchronous use of the old firmware APIs consist of
> +the caller requesting for "driver data" and then finally freeing it on
> +asynchronous callback.
> +
> +The driver data request API enables callers to provide a callback for both
> +synchronous and asynchronous requests and since consumption can be expected
> +in these callbacks it frees it for you by default after callback handlers
> +are issued. If you wish to keep the driver data around after your callbacks
> +you must specify this through the driver data request paramter data structure.
> +
> +Async cookies, replacing completions
> +====================================
> +
> +With this new API you do not need to declare and use your own completions, you

It's not going to be "new" in a year, are you going to go and change the
documentation here?

And if you want to provide a "how to convert from firmware to
driver_data" document, great, but to constantly compare the two seems a
bit like you are trying too hard.  It should stand on it's own without
needing to do that.

> +can replace your completions with drvdata_synchronize_request() using the
> +async_cookie set for you by drvdata_file_request_async(). When
> +drvdata_file_request_async() completes you can rest assured all the work for
> +both triggering, and processing the drvdata using any of your callbacks has
> +completed.
> +
> +Fallback mechanisms on the driver data API
> +==========================================
> +
> +The old firmware API provided support for a series of fallback mechanisms. The
> +new driver data API abandons all current notions of the fallback mechanisms,
> +it may soon add support for one though.

Oh come on, is this paragraph really needed at all?  "soon"?  Hah.


> +Tracking development enhancements and ideas
> +===========================================
> +
> +To help track ongoing development for firmware_class and related items to
> +firmware_class refer to the kernel newbies wiki page [0].
> +
> +[0] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements
> diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
> index 1abe01793031..8d275c4c165b 100644
> --- a/Documentation/driver-api/firmware/index.rst
> +++ b/Documentation/driver-api/firmware/index.rst
> @@ -7,6 +7,7 @@ Linux Firmware API
>     introduction
>     core
>     request_firmware
> +   drvdata
>  
>  .. only::  subproject and html
>  
> diff --git a/Documentation/driver-api/firmware/introduction.rst b/Documentation/driver-api/firmware/introduction.rst
> index 211cb44eb972..d7d5ef846ca0 100644
> --- a/Documentation/driver-api/firmware/introduction.rst
> +++ b/Documentation/driver-api/firmware/introduction.rst
> @@ -25,3 +25,14 @@ are already using asynchronous initialization mechanisms which will not
>  stall or delay boot. Even if loading firmware does not take a lot of time
>  processing firmware might, and this can still delay boot or initialization,
>  as such mechanisms such as asynchronous probe can help supplement drivers.
> +
> +Two APIs
> +========
> +
> +Two APIs are provided for firmware:
> +
> +* request_firmware API - old firmware API
> +* drvdata API - new flexible API

"new" isn't "new" in a few months.

thanks,

greg k-h

Powered by blists - more mailing lists