Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params
To: Greg KH <gregkh@linuxfoundation.org>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl,
        arend.vanspriel@broadcom.com, rjw@rjwysocki.net,
        atull@opensource.altera.com, moritz.fischer@ettus.com,
        pmladek@suse.com, johannes.berg@intel.com, emmanuel.grumbach@intel.com,
        luciano.coelho@intel.com, kvalo@codeaurora.org, luto@kernel.org,
        torvalds@linux-foundation.org, keescook@chromium.org,
        takahiro.akashi@linaro.org, dhowells@redhat.com, pjones@redhat.com,
        hdegoede@redhat.com, alan@linux.intel.com, tytso@mit.edu,
        linux-kernel@vger.kernel.org
References: <20170605213314.GR8951@wotan.suse.de>
 <20170605213937.26215-1-mcgrof@kernel.org>
 <20170605213937.26215-2-mcgrof@kernel.org> <20170613090548.GA31421@kroah.com>
 <20170613194011.GI27288@wotan.suse.de> <20170617193815.GI2974@kroah.com>
From: "Li, Yi" <yi1.li@linux.intel.com>
Message-ID: <b22b90ef-ab65-b077-c146-9f078bbe0b47@linux.intel.com>
Date: Mon, 19 Jun 2017 17:51:08 -0500
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <20170617193815.GI2974@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3880
Lines: 94

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
>