Messages in this thread | | | Date | Sat, 17 Jun 2017 21:38:15 +0200 | From | Greg KH <> | Subject | Re: [PATCH v9 1/5] firmware: add extensible driver data params |
| |
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
| |