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

From: Li, Yi
Date: Mon Jun 19 2017 - 18:51:16 EST

Hi Greg,

On 6/17/2017 2:38 PM, 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 :)

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.

Again, the code is now bigger, does more, with not even any real benefit
for existing users.

I am still new to the upstreaming world, pardon me if my understanding is naive. :) My take with Luis's driver data API is that it adds a wrapper on top of the old request_firmware APIs, so the new features can be added/disabled by the parameters structures instead of adding/changing API functions. Agree that there is not much new for existing users. It adds more codes (not necessary more complex) but create a consistent way for extension IMO.

Below are 3 examples I tried to add streaming support to load large firmware files.
Adding streaming with driver data API: . This patch series depends on
non-cache patch series , which is bigger than it should be since it added some codes to test firmware caching.
and pre-allocate buffer patch series

By comparison, here is my old streaming RFC with original firmware class:
Do you think this is the better way?


If not, what concrete alternatives do you suggest?

It's working, so leave it alone? :)


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. Heck, I'm still not
convinced that firmware signing isn't anything more than just some
snakeoil in the first place!

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

To get me to take these changes, you have to show a real need and user
of the code. 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.

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,


greg k-h