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

Li, Yi
2017-06-19T22:51:13Z

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
>