Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562632 > unrolled thread

Re: [PATCH v4 1/3] firmware: add new extensible firmware API - drvdata

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/3] firmware: add new extensible firmware API -  drvdata Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 12:40 +0100
    Re: [PATCH v4 1/3] firmware: add new extensible firmware API -  drvdata "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-19 18:00 +0100

#1562632 — 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

[toc] | [next] | [standalone]

#1562920

On Thu, Jan 19, 2017 at 12:36:52PM +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 07:02:42AM -0800, Luis R. Rodriguez wrote:
> 
> I think this changelog novel is longer than the documentation you added
> to the kernel :(

Heh, OK will work on that.

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

Sure thing'o.

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

Hm yeah it was not clear I was trying to explain *why* we ended up implementing
a flexible API. Will work on that.

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

Will fix.

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

Sure.

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

As recent fixes for the fallback mechanism show the fallback mechanism
is pretty hairy. I want to clean up all that mess, add respective test
units for it. There are also some more longer term things to consider
which I'd like to address as well, for instance the firmware code is the
only code using the general UMH lock, although it was originally added
to help warn for firmware APi uses on suspend/resume the firmware cache
mechanism now helps resolve the issues -- the only case which cannot take
advantage of this is the custom fallback mechanism. One also needs to
consider if the UMH lock or a replacement should be considered for other
kernel UMH. A related effort here which can also help is a kernel
functionality to help address races with block devices freeze_super() can be
used to queue superblock filesystem operations. Getting this resolved
(as discussed at kernel summit by Jiri) could help resolve some of the corner
case suspend/resume race concerns for to help replace the lock for the
old API (custom fallback mechanism).

There are *real* more practical races on init which we will be able to soon
fix with the new firmwared effort by Daniel Wagner and Tom Gunderson. Once
most of this is lined up I'd feel much more comfortable with a fallback
mechanism.

I really am trying hard to make that fallback mechanism *work* fine, right now
its just hairballs. More patches to follow up to help with that..

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

Will fix.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web