From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752546AbdFSWvN (ORCPT ); Mon, 19 Jun 2017 18:51:13 -0400 Received: from mga14.intel.com ([192.55.52.115]:59629 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752298AbdFSWvL (ORCPT ); Mon, 19 Jun 2017 18:51:11 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,363,1493708400"; d="scan'208";a="115081688" Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params To: Greg KH , "Luis R. Rodriguez" 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" Message-ID: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >