From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033991AbdD2Eg7 (ORCPT ); Sat, 29 Apr 2017 00:36:59 -0400 Received: from mx2.suse.de ([195.135.220.15]:52692 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751531AbdD2Egv (ORCPT ); Sat, 29 Apr 2017 00:36:51 -0400 Date: Sat, 29 Apr 2017 06:36:47 +0200 From: "Luis R. Rodriguez" 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 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 28, 2017 at 12:19:05PM +0900, AKASHI Takahiro wrote: > > > > + 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? 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