Date:   Mon, 26 Jun 2017 20:19:07 +0200
From:   Rafał Miłecki <rafal@...ecki.pl>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Vikram Mulukutla <markivx@...eaurora.org>,
        Stephen Boyd <stephen.boyd@...aro.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Julia Lawall <julia.lawall@...6.fr>,
        Daniel Wagner <wagi@...om.org>,
        David Woodhouse <dwmw2@...radead.org>,
        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 2017-06-26 19:33, Luis R. Rodriguez wrote:
> 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.

I believe there was a misunderstanding of my patch by Hans. The point of 
my
patch was to don't display warning *IF* we can use alternative soruce 
and
get the NVRAM (firmware) from platform data (special partition used by 
the
bootloader and accessible by the operating system).

Powered by blists - more mailing lists