Re: [PATCH v9 1/5] firmware: add extensible driver data params

From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	Daniel Wagner <wagi@monom.org>,
	David Woodhouse <dwmw2@infradead.org>,
	rafal@milecki.pl, Arend Van Spriel <arend.vanspriel@broadcom.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Li, Yi" <yi1.li@linux.intel.com>,
	atull@opensource.altera.com,
	Moritz Fischer <moritz.fischer@ettus.com>,
	Petr Mladek <pmladek@suse.com>,
	Johannes Berg <johannes.berg@intel.com>,
	Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
	"Coelho, Luciano" <luciano.coelho@intel.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	Andrew Lutomirski <luto@kernel.org>,
	Jiri Kosina <jkosina@suse.cz>, Kees Cook <keescook@chromium.org>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	David Howells <dhowells@redhat.com>,
	Peter Jones <pjones@redhat.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Alan Cox <alan@linux.intel.com>, "Theodore Ts'o" <tytso@mit.edu>,
	NeilBrown <neilb@suse.com>, Christoph Hellwig <hch@lst.de>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params
Date: Sat, 24 Jun 2017 02:48:28 +0200	[thread overview]
Message-ID: <20170624004828.GA21846@wotan.suse.de> (raw)
In-Reply-To: <CA+55aFyGBOpeyxkhER+bD0NNiskuFktMDsPo_K=GgV7fZZMAJg@mail.gmail.com>

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@kernel.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.

> It's too indirect, it adds all those nasty "descriptors" of what to
> do, and it doesn't match what the current model does at all.

This is good feedback, I do accept deciding where to draw the line is hard.
I decided to go with blocking/non-blocking as the fine line.

> 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...

> The thing is, it's much better to just have functions that load the
> firmware data. Have them named that way ("load_firmware()"), and act
> that way ("just load the damn firmware file") instead of having odd
> descriptors that describe what is goign to be done and some state for
> it, and then get passed around.
> 
> Don't add this kind of crazy abstraction complexity.
> 
> If somebody wants to veryify a signature on a firmware file, they
> should *NOT* fill in a descriptor that says "check signature when
> loading". Thats' complete BS.
> 
> They should just do "load_firmware()" and then "check_signature()" or whatever.

That's a fair suggestion for firmware signing! And I'll let AKASHI comment on
whether or not that would suffice for his requirements given he's now
addressing firmware signing.

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?

Or let us just consider the very simple *optional* async firmware. Do we add
*one* full new API call just for that?

  Luis