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

["Li, Yi" <yi1.li@linux.intel.com>]

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
>