Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933027AbdD0GKM (ORCPT ); Thu, 27 Apr 2017 02:10:12 -0400 Received: from paleale.coelho.fi ([176.9.41.70]:54880 "EHLO farmhouse.coelho.fi" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932797AbdD0GKD (ORCPT ); Thu, 27 Apr 2017 02:10:03 -0400 Message-ID: <1493273387.14233.13.camel@coelho.fi> From: Luca Coelho To: "Luis R. Rodriguez" Cc: "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , "pjones@redhat.com" , "moritz.fischer@ettus.com" , "takahiro.akashi@linaro.org" , "dhowells@redhat.com" , "pmladek@suse.com" , "Berg, Johannes" , "rjw@rjwysocki.net" , "yi1.li@linux.intel.com" , "kvalo@codeaurora.org" , "luto@kernel.org" , "arend.vanspriel@broadcom.com" , "rafal@milecki.pl" , "dwmw2@infradead.org" , "wagi@monom.org" , "atull@opensource.altera.com" , "Grumbach, Emmanuel" Date: Thu, 27 Apr 2017 09:09:47 +0300 In-Reply-To: <20170427031625.GI28800@wotan.suse.de> References: <20170330032514.17173-1-mcgrof@kernel.org> <20170330032514.17173-3-mcgrof@kernel.org> <1491828162.31980.50.camel@intel.com> <20170427031625.GI28800@wotan.suse.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 91.156.4.241 X-SA-Exim-Mail-From: luca@coelho.fi Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API X-SA-Exim-Version: 4.2.1 (built Tue, 02 Aug 2016 21:08:31 +0000) X-SA-Exim-Scanned: Yes (on farmhouse.coelho.fi) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2268 Lines: 55 On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote: > > > +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; > > > > Why do you need to check this here? If the caller is calling _sync(), > > it's because that's what it needs.  This mode value here seems > > redundant. > > Because we allow one data structure to be used for the specified requirements > and technically someone can make a mistake and use the wrong macros to set up > the data structure. This ensures users don't async macros to set up the > parameters and then use the sync call. Eventually the underlying > firmware_class.c code does its own conditional checks on this as well. If this could only happen in a programming error, maybe it's worth a WARN() then, to make it easier to spot? > > OTOH, if you do have a reason for this value, then you could use > > driver_data_request_sync() in this if. > > You mean to allow *one* API call for all. Sure, that's possible, but I think > its nicer to at least expose async/sync mechanisms on the caller side. The > sync/async differences seem like a reasonable enough place to split the API. I don't remember the details of this anymore, but doesn't the driver_data_sync() function does exactly the same check? I meant that you could do this: if(WARN_ON_ONCE(!driver_data_request_sync())) return -EINVAL; And yes, I think either a single API call for all or not having the mode in the struct would be cleaner. I think there are better ways to prevent coding errors (since this should only happen if the user code is implemented incorrectly). -- Cheers, Luca.