Date: Fri, 28 Apr 2017 02:51:44 +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: <20170428005144.GM28800@wotan.suse.de>
References: <20170330032514.17173-1-mcgrof@kernel.org>
 <20170330032514.17173-3-mcgrof@kernel.org>
 <20170413093613.GF15139@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170413093613.GF15139@linaro.org>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4205
Lines: 114

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