Date: Sat, 29 Apr 2017 06:36:47 +0200
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: AKASHI Takahiro <takahiro.akashi@linaro.org>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>, gregkh@linuxfoundation.org,
        wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl,
        arend.vanspriel@broadcom.com, rjw@rjwysocki.net,
        yi1.li@linux.intel.com, atull@opensource.altera.com,
        moritz.fischer@ettus.com, pmladek@suse.com, johannes.berg@intel.com,
        emmanuel.grumbach@intel.com, luciano.coelho@intel.com,
        kvalo@codeaurora.org, luto@kernel.org, dhowells@redhat.com,
        pjones@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API
Message-ID: <20170429043647.GQ28800@wotan.suse.de>
References: <20170330032514.17173-1-mcgrof@kernel.org>
 <20170330032514.17173-3-mcgrof@kernel.org>
 <20170413093613.GF15139@linaro.org>
 <20170428005144.GM28800@wotan.suse.de>
 <20170428031904.GA5123@fireball>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170428031904.GA5123@fireball>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3255
Lines: 66

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