Message-ID: <20170626173328.GC21846@wotan.suse.de>
Date:   Mon, 26 Jun 2017 19:33:28 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Vikram Mulukutla <markivx@...eaurora.org>,
        Stephen Boyd <stephen.boyd@...aro.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Julia Lawall <julia.lawall@...6.fr>,
        Daniel Wagner <wagi@...om.org>,
        David Woodhouse <dwmw2@...radead.org>, rafal@...ecki.pl,
        Arend Van Spriel <arend.vanspriel@...adcom.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Li, Yi" <yi1.li@...ux.intel.com>, atull@...nsource.altera.com,
        Moritz Fischer <moritz.fischer@...us.com>,
        Petr Mladek <pmladek@...e.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        "Coelho, Luciano" <luciano.coelho@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Kees Cook <keescook@...omium.org>,
        "AKASHI, Takahiro" <takahiro.akashi@...aro.org>,
        David Howells <dhowells@...hat.com>,
        Peter Jones <pjones@...hat.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Alan Cox <alan@...ux.intel.com>, Theodore Ts'o <tytso@....edu>,
        NeilBrown <neilb@...e.com>, Christoph Hellwig <hch@....de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params

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 <mcgrof@...nel.org> 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

Powered by blists - more mailing lists