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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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 <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 +
> > 
> > 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, &params, 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