Messages in this thread | | | Subject | Re: [PATCH v9 1/5] firmware: add extensible driver data params | From | "Li, Yi" <> | Date | Mon, 19 Jun 2017 17:51:08 -0500 |
| |
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: https://patchwork.kernel.org/patch/9738503 . This patch series depends on non-cache patch series https://patchwork.kernel.org/patch/9793825 , which is bigger than it should be since it added some codes to test firmware caching. and pre-allocate buffer patch series https://patchwork.kernel.org/patch/9738487/
By comparison, here is my old streaming RFC with original firmware class: https://lkml.org/lkml/2017/3/9/872 Do you think this is the better way?
Thanks, Yi
> >> 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 >
| |