Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754407AbdFWWnp (ORCPT ); Fri, 23 Jun 2017 18:43:45 -0400 Received: from mx2.suse.de ([195.135.220.15]:44235 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752606AbdFWWno (ORCPT ); Fri, 23 Jun 2017 18:43:44 -0400 Date: Sat, 24 Jun 2017 00:43:38 +0200 From: "Luis R. Rodriguez" To: Greg KH , Julia Lawall Cc: "Luis R. Rodriguez" , 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, jkosina@suse.cz, torvalds@linux-foundation.org, keescook@chromium.org, takahiro.akashi@linaro.org, dhowells@redhat.com, pjones@redhat.com, hdegoede@redhat.com, alan@linux.intel.com, tytso@mit.edu, NeilBrown , Christoph Hellwig , linux-kernel@vger.kernel.org Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params Message-ID: <20170623224338.GX21846@wotan.suse.de> References: <20170605213314.GR8951@wotan.suse.de> <20170605213937.26215-1-mcgrof@kernel.org> <20170605213937.26215-2-mcgrof@kernel.org> <20170613090548.GA31421@kroah.com> <20170613194011.GI27288@wotan.suse.de> <20170617193815.GI2974@kroah.com> <20170619193522.GH21846@wotan.suse.de> <20170623155123.GB3565@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170623155123.GB3565@kroah.com> 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 Content-Length: 5786 Lines: 109 On Fri, Jun 23, 2017 at 11:51:23PM +0800, Greg KH wrote: > On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote: > > > I agree, that's what I'm saying here. I just do not see that happening > > > with your patch set at all. It's adding more code, a more complex way > > > to interact with the subsystem, and not making driver writer lives any > > > easier at all that I can see. > > > > There are two things to consider: > > > > a) The current design of the firmware API, and interfaces with exported symbols > > > > The case for the driver data API was that we were being super sloppy with extensions, > > to the point was making the internal code base very bug prone and full or redirect > > conditionals with #ifdefery nightmware stuff. > > > > b) Features of the firmware API > > > > These have to be evaluated on a case by case basis. > > Wait, no, you didn't address my main complaint at all here. You are > adding complexity for no perceived gain at all with this patch set. > > Now you might feel that this series gets you moving forward toward an > end goal of reduced complexity and wonderfulness, but you know how > kernel development works, you have to justify _all_ of your changes, not > just some future end result that is not even presented here. My point was that a) was already complex, so to say what I'm adding to address a) is complex would be unfair, so rather the question should be if its less complex, or if there are valid technical issues I'd like to hear them. > > > I, and others I know, have told you to work on simplifying your > responses, and descriptions, of patches. Take the extra time to make a > shorter answer. You will get better results, as I dread having to read > and respond to them currently. Sure, point taken! > I know you have spent a lot of time and effort on this work, but as it > stands, this crazy new interface (data-driven api vs. the traditional > procedural apis we know and love in Linux), is not acceptable at all. Ah, yes! Here is what I believe seems to be the *crux* issue of these patch series and I'm happy we have finally landed on it. Yes, indeed the new API proposed here provides more flexibility, and it does so by embracing a "data driven" API Vs the traditional procedural APIs we have seen for *the firmware API*. If by data-driven you mean using structs to drive the requirements, instead of adding more API per new feature. I would strongly disagree that we always prefer adding new functional APIs loosely. I think this should be reviewed on a case by case basis and it should be up to the maintainer who has better visibility into the history of the code, and what is coming. A quick git log grep found a recent commit example where we take on a flexible API on other *random* places in Linux. Refer to for example the change form bioset_create_nobvec() to bioset_create() and use flags in the patch titled "blk: replace bioset_create_nobvec() with a flags arg to bioset_create()", present on linux-next [0] followed by "blk: make the bioset rescue_workqueue optional." [1] which also extends the flags and uses them. To argue that we *still* need to keep doing a functional approach for the firmware API and keep adding new routines for new features, seems insane to me at this point -- if that is what you were suggesting... Also, the reason *why* I think this discussion is important is that it also implicates the amount of collateral evolutions needed per API. If you embrace data-driven API (or flags, or structs for APIs) you should see less patches due to collateral evolutions. In a way its my own resolution to mitigate *unnecessary* collateral evolutions by proper architecture. Where that line is drawn should be up to the designers of the API. For system calls, for instance, I think its well accepted we *never* want to make inflexible APIs. There is strong history for why that is the case. We cannot compare system calls to exported symbols, at all, but we can learn a bit from the flexibility efforts on them for exported symbols as well. Its *real news* to me that we *always* prefer a procedural preferences for APIs / exported symbols. [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=011067b05668b05aae88e5a24cff0ca0a67ca0b0 [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=47e0fb461fca1a68a566c82fcc006cc787312d8c > It's also blocking real bug fixes and features that people want > addressed, which isn't acceptable. What? What bugs ? I have addressed *every single* stable issue in queue and have sent patches for them and they do not depend in any way on any of this series! In fact I've taken liberty to go to great lengths to ensure *stable* proposals *get proper review* so we *do the right thing*. Case in point was the recent -ERESTART crap which in the end we ended up moving towards preferring adding new swait API for stable for it. I have ZERO stable fixes pending on my queue! Please name the bug pending. In so far as features.. yes ! This delays adding new features because the crux of the discussion is *how* to add them, a flag or new parameter in a struct *or* do we add yet-another-API-call? I'm making a case for the first. > Please take the time to step back, and see if you really want to spend > the effort into creating something that you can easily justify and break > down into acceptable patches. If so, great, do it, but as it stands > today, that is not what you have done here, at all. AKASHI addressed how to break down the patches further. I will take that on! Your point above on data-driven Vs functional however still deserves some discussion as otherwise the effort on the changes I've made are pointless. Luis