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

Groups > linux.kernel > #1632466 > unrolled thread

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

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 v6 2/5] firmware: add extensible driver data API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-04-28 03:00 +0200
    Re: [PATCH v6 2/5] firmware: add extensible driver data API AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-04-28 05:20 +0200
      Re: [PATCH v6 2/5] firmware: add extensible driver data API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-04-29 06:40 +0200

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

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

> 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

[toc] | [next] | [standalone]

#1632513

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

[toc] | [prev] | [next] | [standalone]

#1633240

On Fri, Apr 28, 2017 at 12:19:05PM +0900, AKASHI Takahiro wrote:
> > > > +	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?

That's right, if you want that feature you must use a sync callback. Some drivers
have the form that they just copy over the data andr elease immediately. Case
in point see the iwlwifi driver which I converted, managing the releases means
also less errors on part of the driver developer on their error paths, and less
code.

> In async case, I think that we should have a callback whether asynchronous
> loader has succeeded or failed in order to know the result.

There are async requests which are completely optional, but indeed even so if no
callback is set then all that would be done is to check if the file exists, so
I agree with you. I will add a respective check forcing for the async callback.

> It will never be "optional" even on failure.

The driver data may be optional but indeed processing it should not be. Come to think
of it the async case also does not give back the return value so for both async and
sync case we should pass the return value to enable the caller to manage different
failures better.

Will modify both callbacks.

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

Historically we just passed NULL on the async callback on return, and the sync case
always got the actual return value. I think we want the return value in failure other
than just NULL. Will make these adjustments.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web