Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S939721AbdD1DTV (ORCPT ); Thu, 27 Apr 2017 23:19:21 -0400 Received: from mail-pg0-f48.google.com ([74.125.83.48]:35139 "EHLO mail-pg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754183AbdD1DTM (ORCPT ); Thu, 27 Apr 2017 23:19:12 -0400 Date: Fri, 28 Apr 2017 12:19:05 +0900 From: AKASHI Takahiro To: "Luis R. Rodriguez" Cc: 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: <20170428031904.GA5123@fireball> Mail-Followup-To: AKASHI Takahiro , "Luis R. Rodriguez" , 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 References: <20170330032514.17173-1-mcgrof@kernel.org> <20170330032514.17173-3-mcgrof@kernel.org> <20170413093613.GF15139@linaro.org> <20170428005144.GM28800@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170428005144.GM28800@wotan.suse.de> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4961 Lines: 130 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 > > > --- > > > 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, ¶ms, 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