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

From	"Li, Yi" <yi1.li@linux.intel.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v9 1/5] firmware: add extensible driver data params
Date	2017-06-20 01:00 +0200
Message-ID	<tUdOh-m9-3@gated-at.bofh.it> (permalink)
References	(1 earlier) <tP7Tb-8dq-7@gated-at.bofh.it> <tP7Tb-8dq-3@gated-at.bofh.it> <tRPZL-65q-11@gated-at.bofh.it> <tRZZ7-3Du-1@gated-at.bofh.it> <tTwgi-6oU-17@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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
>

Thread

Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-13 11:10 +0200
  Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-13 13:10 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-13 15:20 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-13 16:50 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-13 17:40 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-13 18:00 +0200
  Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-13 21:50 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-14 18:00 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-18 02:30 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Johannes Berg <johannes@sipsolutions.net> - 2017-06-19 09:40 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-19 21:50 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-20 03:30 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-19 21:40 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-23 22:30 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-24 00:50 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-24 01:10 +0200
              Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-24 02:50 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-24 14:50 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 19:40 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-26 22:50 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 23:40 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params Vikram Mulukutla <markivx@codeaurora.org> - 2017-06-27 04:30 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 19:30 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-24 14:50 +0200
              Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 18:00 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-23 22:30 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-24 00:50 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-20 01:00 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-20 03:50 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-20 17:30 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Vikram Mulukutla <markivx@codeaurora.org> - 2017-06-20 18:30 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-20 19:30 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-21 03:00 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-23 18:40 +0200

csiph-web