Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752951AbdFSTf3 (ORCPT ); Mon, 19 Jun 2017 15:35:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:47492 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752415AbdFSTf0 (ORCPT ); Mon, 19 Jun 2017 15:35:26 -0400 Date: Mon, 19 Jun 2017 21:35:22 +0200 From: "Luis R. Rodriguez" To: Greg KH 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, 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, linux-kernel@vger.kernel.org Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params Message-ID: <20170619193522.GH21846@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170617193815.GI2974@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: 10988 Lines: 221 On Sat, Jun 17, 2017 at 09:38:15PM +0200, Greg KH wrote: > On Tue, Jun 13, 2017 at 09:40:11PM +0200, Luis R. Rodriguez wrote: > > On Tue, Jun 13, 2017 at 11:05:48AM +0200, Greg KH wrote: > > > On Mon, Jun 05, 2017 at 02:39:33PM -0700, Luis R. Rodriguez wrote: > > > > As the firmware API evolves we keep extending functions with more arguments. > > > > Stop this nonsense by proving an extensible data structure which can be used > > > > to represent both user parameters and private internal parameters. > > > > > > Let's take a simple C function interface and make it a more complex > > > data-driven interface that is impossible to understand and obviously > > > understand how it is to be used and works! > > > > The firmware codebase was already complex! > > Heh, I'm not arguing with you there :) Great! > > What you have to ask yourself really is if this makes it *less complex* and > > helps *clean things up* in a much better way than it was before. Also does it > > allow us to *pave the way for new functionality easily*, without creating > > further mess? > > 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. > Again, the code is now bigger, does more, with not even any real benefit > for existing users. Obviously I disagree strongly, in light of the history of the code. Not only should the existing code be compared but also how it has evolved and we should evaluate whether or not its evolution can be dealt with more appropriately. > > If not, what concrete alternatives do you suggest? > > It's working, so leave it alone? :) As the maintainer of the firmware API I **totally** disagree!! First the firmware API has been evolving as pieces of gum thrown at it. A proper design of the API in consideration of extensions is in order. Likewise goes for testing and documentation. Second, it cannot be said with any serious tone that the current design has worked well. It is simply not an accurate reflection of the codebase. The design of the fallback mechanism and the amount of issues that have gone through it is a great example. The issues I have fixed along the way, and the chaotic way in which the API has evolved have put me to reflect well on a proper design of the firmware API. > > > :( > > > > > > Seriously, why? Why are we extending any of this at all? > > > > Addressing easy extensibility was a prerequisite of considering firmware signing > > support. Its really the *only* reason I started reviewing the firmware API, to the > > point I started helping fix quite a bit of bugs and now just became the maintainer. > > > > So my original firmware signing effort, now driven by AKASHI Takahiro, was one > > of the original motivations here! > > But we don't accept kernel patches for some mythical future option that > might be happening some time in the future. Greg, these are non mythical "future options" -- they are real feature requests and they have all valid reasons and discussions ongoing on the mailing list. The streaming FPGA support is a concrete valid use case we need to extend *now* and your reply brilliantly seems to have completely ignored it. I do agree that firmware signing itself was a largely debatable topic, there were *really-really* long threads on the topic. However we had a hallway track at Santa Fe Plumbers where it seemed we reached consensus on a path forward! The firmware signing topic should be addressed on TAKASHI's patch set [0]. Please address your NACK and reasons there. Even though it is based on the driver data API, the main topic points of *logic* for it can *also* be discussed there. BUT NOTE: Before we could move forward with firmware signing we need also a *sane* way to extend the API to address the needs of firmware signing. So a proper sane API for future extensions is in order as a prerequisite. This goes along with testing and documentation. This goes for *any* new firmware API feature!!! Firmware signing was just one example feature!!! Also -- even if they were "mythical" the *point* of this series is to address extensibility of the API, and so one of the ways to measure the gains of the design of a new API is to look at what the code would look like when new features are added. This is why I pointed out to review these proposed changes. Even more so given I am noting that these are non-mythical features!! [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org > Heck, I'm still not convinced that firmware signing isn't anything more than > just some snakeoil in the first place! Sure, some may have similar sentiments, this topic should be addressed separately on TAKASHI's latest patches [0]. Please address this on the patches posted, Otherwise I really am afraid we would conflating discussions and confusing the core issue on this thread: The future direction of extensibility of the firmware API! > So while you mention lots of times that all sorts of wonderful things > can now possibly be built on top of the new code, I have yet to see it > (meaning you didn't include it in the patch series.) Clearly I disagree, but perhaps the issue here may be conflating "addressing extensibility of the firmware API" with actual features. > To get me to take these changes, you have to show a real need and user > of the code. The *core issue* here seems to be that the features for which we have had to consider new extensions of the firmware API have been debatable, so my arguments for proper design are being conflated with the features introduced at the time the redesign is introduced. The driver data API was originally introduced with firmware signing, and the topic of firmware signing alone was a hugely debatable topic. Even though I note those debatable issues were resolved its fair to still question them, but those must be addressed separately. For this reason I looked at the next feature on my radar to consider which we needed which was non-debatable. The next closest feature was that of making async firmware requests optional, just as request_firmware_direct() exist for sync requests. Unfortunately the *need* for this *just disappeared* given that it was clarified by Hans de Geode that the driver that *needed* this, brcmfmac, no longer optionally needs the firmware [1]. This is precisely why I dropped the respective brcmfmac changes in my last iteration of the driver data API. Even though I dropped the actual driver use, I kept the feature as its obviously sensible! But anyway, my point is that this *reason* vanished from upstream all of a sudden... Next, as the firmware maintainer, I knew addressing daisy chained requests for a series of firmware revisions was another *good idea* to address upstream properly, given this can be complex and the amount of issues that could lurk there. You have foo driver which supports a series of ranges of API firmware X-Y, we should allow for an API which enables this in a easy way. A good example complex case was in the iwlwifi driver, it actually uses recursion to accomplish the above objectives. Despite the recursion design, their layout for expectations for the file names for a series of API revisions seems very sensible to me. Its a flexible layout I think we can stick to and support generically. As such I implemented this on the firmware API and made the implementation deterministic, avoiding recursion. This feature alone should be weighed on its own. While I do agree it would seem to appear we only have *one upstream* user right now, I cannot imagine that this is generally true, daisy chaining requests on a series of revisions seems to me a logical approach and I expect much more users may exist, its just a matter hunting and conversion. You may argue that *one* upstream users is not sufficient to introduce a new feature for, but I disagree given we have had new full *API* added for a new feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For instance request_firmware_into_buf() has no upstream users!!! Now, you might say that even though this is true that there many users of out-of-tree drivers that need this. While true, if this is the bar we'd go with, we can't then ignore the iwlwifi userbase, and the possible gains of having a proper non-recursive use of the daisy chained requests. Also, its *precisely* this loose API garbage such as what went in with request_firmware_into_buf() which I'm trying to avoid. Loosely adding API with hidden features is *not a good idea*, can lead to further misprogramming or odd bugs. Worst it also paves the way to further sloppy extensions. For instance the request_firmware_into_buf() API makes use of a no-cache feature which we *also* can have a use for on *existing* drivers upstream such as iwlwifi which deal with *caching on its own* ! Its also a requirement for streaming API for FPGAs. Also it should be considered that other than addressing the extensibility of the firmware API, the driver data API intentionally strives to bundle specific unit tests for *each* feature being introduced. This also goes with a philosophy re-design of the test driver: instead of making *one* knob per test case in the C test driver, I am designing the test case configuration completely in userspace, striving to allow us to build *any* possible test case form userspace. [1] https://lkml.kernel.org/r/09063fc2-af77-ced6-ed90-ab20e2884969@redhat.com > Without that it just strongly looks like you are having > fun making a more complex api for no reason that we are then going to be > stuck with maintaining. Greg, this is rather insulting considering the amount of work I have put into thought about proper design of the firmware API in consideration of past issues and its rather loose grotesque evolution which I have been alerting to and pointing out. > So clean away, and fix up, but remember, you have to be able to justify > each change as being needed. And so far, I'm not sold on this at all, > sorry. I have put a lot of work into a proper design here in consideration of the entire history of the firmware API. We *need* a proper architectural design for the firmware API to evolve it in ways that does not allow folks to just throw gum at it. We also want to build test units for each new feature and ensure *each new feature* is properly documented. That is what I have provided in this patch series. Luis