Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751469AbdFZRdj (ORCPT ); Mon, 26 Jun 2017 13:33:39 -0400 Received: from mx2.suse.de ([195.135.220.15]:58257 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750853AbdFZRdc (ORCPT ); Mon, 26 Jun 2017 13:33:32 -0400 Date: Mon, 26 Jun 2017 19:33:28 +0200 From: "Luis R. Rodriguez" To: Greg KH , Vikram Mulukutla , Stephen Boyd Cc: "Luis R. Rodriguez" , Linus Torvalds , Julia Lawall , Daniel Wagner , David Woodhouse , rafal@milecki.pl, Arend Van Spriel , "Rafael J. Wysocki" , "Li, Yi" , atull@opensource.altera.com, Moritz Fischer , Petr Mladek , Johannes Berg , Emmanuel Grumbach , "Coelho, Luciano" , Kalle Valo , Andrew Lutomirski , Jiri Kosina , Kees Cook , "AKASHI, Takahiro" , David Howells , Peter Jones , Hans de Goede , Alan Cox , "Theodore Ts'o" , NeilBrown , Christoph Hellwig , Linux Kernel Mailing List Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params Message-ID: <20170626173328.GC21846@wotan.suse.de> References: <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> <20170623224338.GX21846@wotan.suse.de> <20170624004828.GA21846@wotan.suse.de> <20170624123951.GA10622@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170624123951.GA10622@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: 6503 Lines: 129 On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: > On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: > > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: > > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez wrote: > > > > > > > > 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*. > > > > > > This has been going on forever. Everybody hates your data-driven one. > > > > Before you, the only person who had expressed disdain here was Greg. > > Very few people actually review code, you know that. Using that logic, then of course "everybody" was *very* fitting ;) Then again others who actually are working on extending the firmware API (Yi Li), or maintaining vendor trees (Vikram), did express their opinions on the current codebase and their appreciate for the changes I made, however this went selectively unnoticed. > > > Things like that may be ok as an internal implementation, but even > > > there it's questionable if it then means a big disconnect between what > > > people actually use (the normal functional model) and the > > > implementation. > > > > A vendor tree implemented their *own* solution and were willing to maintain > > it despite this likely making it hard to port stable fixes. That I think says > > a lot for a need... > > What vendor tree? Where was it shipped? The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could mean millions of devices. https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319 > Why was it external and how is it different from your patches? As is typical with external trees -- it would seem Vikram actually wrote the original request_firmware_into_buf() API for the msm tree. It contained the fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming effort and he dropped that fw_desc effort from the upstreaming effort. Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd as I am with you on this thread back when request_firmware_into_buf() was being upstreamed [0]. He noted that back then reason for this proposed change was that "the number of things being passed around between layers of functions inside firmware_class seemed a bit untenable". I will note around that time I had proposed a similar change using the fw_desc name, it was only later that this renamed to a params postfix as Linus did not like the descriptor name. [0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org The only difference is that his patch does only modifying the private members of the internal API and routines from my patch 1/5, and he kept the "descriptor" name Linus disliked a while ago. This is precisely why AKASHI noted I could split up my patch 1 in more ways in this series to help *patch review*. > Was it used because your version has taken so long to be submitted/reviwed? Vikram would have a better idea as he is the one who authored it, but it would seem this effort was in parallel to my own at that time. > > There are still other requirements and features in the pipeline for which we > > can consider parameters to parse for, rather than adding new API. Case in > > point, do we want *one* API just to disable the firmware cache? Specially > > knowing that another feature in the pipeline later would make use of this as a > > requirement? > > Again, I do not care! You can not justify patches today with some > mythical thing in the future that might never even happen. Some of these features are things actually being discussed for a while, so to say they are mythical is not accurate. I can trace back firmware signing discussions back to 2015, along with Plumbers in person discussions where we seem to have agreed upon a path forward among a few folks who disagreed on a technical basis. Linaro has a clear interest so AKASHI picked up that work now as I have been busy with general maintainer duties. The fact that Linus just suggested an alternative approach to a params approach is new, and yet to be reviewe by AKASHI for firmware signing. Granting the option to make async firmware optional was discussed since December 2016 by Rafa? [1]. It was only later during my driver data API changes that Hans noted the nvram part was actually *not* optional [2] so this requirement dropped. *However* as the maintainer I believ ethis requirement *is sensible* and would not be surprised if alternative firmware already exists where this is what is intended. The streaming support for FPGAs has been going through a round of reviews since March [3]. The fact that you only become aware or jump into review now does not make them mythical. [1] https://lkml.kernel.org/r/CACna6rxOGo0e9U7eXpUgnnBuxL+x1B0JBf9ZBq2WPbaBE=YZ-g@mail.gmail.com [2] https://lkml.kernel.org/r/09063fc2-af77-ced6-ed90-ab20e2884969@redhat.com [3] https://lkml.kernel.org/r/1489105090-4996-1-git-send-email-yi1.li@linux.intel.com > Again, as it stands, this patch series is unacceptable, and the added > complexity of a crazy api that goes against almost all normal in-kernel > apis, is only one part of the reason. Back to the *real* highlight and *crux* of this thread: This a paradigm position, and I'm fine to go with it! I sure hope my original logic here is not forgotten, the goal was to demo and show an API which mitigates unnecessary collateral evolutions. I'll also note an *alternative* has not yet been clearly suggested, so I'd be *delighted* to hear alternatives. > The other being the loads of added code for no apparent benifit at all. I can split up patches as AKASHI suggested. > So please both fix the api to be "normal", Will do, but an alternative to the approach would be appreciated. Take the optional firmware for async requests as a good example to start with. > and show as to why these > patches are actually needed _today_, otherwise we can just live with > what we have now just fine and muddle along like always. I think a cleanup for the internal API, flags, and things shuffling back and forward makes sense already so will start off with *just that*. Luis