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

[Li, Yi]

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