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

[Greg KH]

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.

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

thanks,

greg k-h