by Luca Coelho

[permalink] [raw]

Subject: Re: [PATCH v6 4/5] iwlwifi: convert to use driver data API

On Fri, 2017-04-28 at 02:56 +0200, Luis R. Rodriguez wrote:
> On Mon, Apr 10, 2017 at 04:19:12PM +0300, Luca Coelho wrote:
> > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> > > The driver data API provides support for looking for firmware
> > > from a specific set of API ranges, so just use that. Since we
> > > free the firmware on the callback immediately after consuming it,
> > > this also takes avantage of that feature.
> > >
> > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > > ---
> >
> > Looks fine, with one nitpick.
> >
> >
> > > drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 67 ++++++++++------------------
> > > 1 file changed, 23 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > index be466a074c1d..b6643aa5b344 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> >
> > [...]
> >
> > > @@ -1541,11 +1522,9 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
> > > }
> > > #endif
> > >
> > > - ret = iwl_request_firmware(drv, true);
> > > - if (ret) {
> > > - IWL_ERR(trans, "Couldn't request the fw\n");
> > > + ret = iwl_request_firmware(drv);
> > > + if (ret)
> > > goto err_fw;
> > > - }
> >
> > Why remove the error message here?
>
> The driver data API now has enough semantics even for async requests so that
> an error is either always issued or supressed (optional is true), driver errors
> then are superfluous on error now.
>
> Let me know if this is OK.

Yeah, that's okay. We can always add something back if we think it's
needed.

--
Cheers,
Luca.

2017-04-29 04:36:59

by Luis Chamberlain

[permalink] [raw]

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

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