Message-ID: <20170619193522.GH21846@wotan.suse.de>
Date:   Mon, 19 Jun 2017 21:35:22 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, wagi@...om.org,
        dwmw2@...radead.org, rafal@...ecki.pl,
        arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        yi1.li@...ux.intel.com, atull@...nsource.altera.com,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org, luto@...nel.org,
        torvalds@...ux-foundation.org, keescook@...omium.org,
        takahiro.akashi@...aro.org, dhowells@...hat.com, pjones@...hat.com,
        hdegoede@...hat.com, alan@...ux.intel.com, tytso@....edu,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params

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

Powered by blists - more mailing lists