Luis,
On Fri, Apr 28, 2017 at 02:51:44AM +0200, Luis R. Rodriguez wrote:
> On Thu, Apr 13, 2017 at 06:36:17PM +0900, AKASHI Takahiro wrote:
> > On Wed, Mar 29, 2017 at 08:25:11PM -0700, Luis R. Rodriguez wrote:
> > > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > > ---
> > > Documentation/driver-api/firmware/driver_data.rst | 77 +++++
> > > Documentation/driver-api/firmware/index.rst | 1 +
> > > Documentation/driver-api/firmware/introduction.rst | 16 +
> >
> > I think we'd better to split code and documents into different patches
> > for easier reviews.
>
> Sure, done.
>
> > > --- a/Documentation/driver-api/firmware/introduction.rst
> > > +++ b/Documentation/driver-api/firmware/introduction.rst
> > > @@ -25,3 +25,19 @@ 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
> > > +* driver_data API - flexible API
> >
> > You can add links:
> >
> > * `request_firmware API`_ - old firmware API
> > * `driver_data API`_ - flexible API
> >
> > .. _`request_firmware API`: ./request_firmware.rst
> > .. _`driver_data API`: ./driver_data.rst
>
> Done!
>
> > > +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;
> > > +
> > > + if (driver_data_sync_opt_cb(req_params) &&
> > > + !driver_data_param_optional(req_params))
> > > + return -EINVAL;
> > > +
> > > + sync_reqs = &dfl_sync_reqs;
> > > +
> > > + __module_get(sync_reqs->module);
> > > + get_device(device);
> > > +
> > > + ret = _request_firmware(&driver_data, name, ¶ms, device);
> > > + if (ret && driver_data_param_optional(req_params))
> > > + ret = driver_data_sync_opt_call_cb(req_params);
> > > + else
> > > + ret = driver_data_sync_call_cb(req_params, driver_data);
> >
> > It looks a bit weird to me that a failure callback is called
> > only if "optional" is set. I think that it makes more sense
> > that a failure callback is always called if _request_firmware() fails.
>
> Let's think about this: does it make sense for the there to be a callback
> if the file was not optional ? Keep in mind the optional flag has its own
> semantics, it affects printing on error, on file not found. The semantics
> of the "optional callback" is precisely defined for when the first file
> is optional, so its by definition.
>
> If we were to not require optional then it would be more of a "failure
> callback",
> as you put it, but then folks could be stuffing these with all their error
> paths, and that's not what this is for. The optional callback is to handle
> an alternative *viable* approach *iff* the first file we look for is not
> found.
In sync case, I don't think we have a strong reason to have a callback
as we can do anything depending on a return value from _request_firmware().
The only merit would be that we could release buffers automatically?
In async case, I think that we should have a callback whether asynchronous
loader has succeeded or failed in order to know the result.
It will never be "optional" even on failure.
> > In addition, why not always return a return value of _request_firmare()?
> > Overwriting a return value by any of callback functions doesn't make sense,
> > particularly, in "sync" case.
> > One of the problems in this implementation is that we, drivers, have
> > no chance to know a return value of _request_firmware().
>
> Ah, good point, well, we can pass it on the optional callback then, this
> way no information is lost.
>
> Thoughts?
Depends on the discussion above.
Thanks,
-Takahiro AKASHI
> > For example, if the signature verification, which I'm now working on, fails,
> > ENOKEY or EKEYxxx will be returns. We may want to say more detailed
> > error messages depending on error code.
>
> Makes sense. If the above suffices let me know.
>
> > > struct driver_data_req_params {
> > > bool optional;
> > > + bool keep;
> > > + bool uses_api_versioning;
> >
> > Do you have any reason that you don't use bit fields here?
> > More features are added, more 'boolean' are added.
> > (I mean it wastes memory.)
>
> You're right, will fold into a flags.
>
> Luis
