Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751754AbdF0C2W (ORCPT ); Mon, 26 Jun 2017 22:28:22 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:37986 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751453AbdF0C2P (ORCPT ); Mon, 26 Jun 2017 22:28:15 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 26 Jun 2017 19:28:12 -0700 From: Vikram Mulukutla To: "Luis R. Rodriguez" , Greg KH , Stephen Boyd Cc: Linus Torvalds , Julia Lawall , Daniel Wagner , David Woodhouse , rafal@milecki.pl, Arend Van Spriel , "Rafael J. Wysocki" , "Li, Yi" , atull@opensource.altera.com, Moritz Fischer , Petr Mladek , Johannes Berg , Emmanuel Grumbach , "Coelho, Luciano" , Kalle Valo , Andrew Lutomirski , Jiri Kosina , Kees Cook , "AKASHI, Takahiro" , David Howells , Peter Jones , Hans de Goede , Alan Cox , "Theodore Ts'o" , NeilBrown , Christoph Hellwig , Linux Kernel Mailing List Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params In-Reply-To: <20170626173328.GC21846@wotan.suse.de> References: <20170605213937.26215-2-mcgrof@kernel.org> <20170613090548.GA31421@kroah.com> <20170613194011.GI27288@wotan.suse.de> <20170617193815.GI2974@kroah.com> <20170619193522.GH21846@wotan.suse.de> <20170623155123.GB3565@kroah.com> <20170623224338.GX21846@wotan.suse.de> <20170624004828.GA21846@wotan.suse.de> <20170624123951.GA10622@kroah.com> <20170626173328.GC21846@wotan.suse.de> Message-ID: <58a06c12-dbba-8a05-1786-464deec0f090@codeaurora.org> User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4477 Lines: 114 On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote: > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote: >> On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote: >>> On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote: >>>> On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez >>>> wrote: >>>>> >>>>> Ah, yes! Here is what I believe seems to be the *crux* issue of >>>>> these patch >>>>> series and I'm happy we have finally landed on it. Yes, indeed the >>>>> new API >>>>> proposed here provides more flexibility, and it does so by >>>>> embracing a >>>>> "data driven" API Vs the traditional procedural APIs we have seen >>>>> for >>>>> *the firmware API*. >>>> >>>> This has been going on forever. Everybody hates your data-driven >>>> one. >>> >>> Before you, the only person who had expressed disdain here was Greg. >> >> Very few people actually review code, you know that. > > Using that logic, then of course "everybody" was *very* fitting ;) > > Then again others who actually are working on extending the firmware > API (Yi > Li), or maintaining vendor trees (Vikram), did express their opinions > on the > current codebase and their appreciate for the changes I made, however > this went > selectively unnoticed. > >>>> Things like that may be ok as an internal implementation, but even >>>> there it's questionable if it then means a big disconnect between >>>> what >>>> people actually use (the normal functional model) and the >>>> implementation. >>> >>> A vendor tree implemented their *own* solution and were willing to >>> maintain >>> it despite this likely making it hard to port stable fixes. That I >>> think says >>> a lot for a need... >> >> What vendor tree? Where was it shipped? > > The msm-3.18 kernel [0], so assuming this goes to mobile devices, this > could > mean millions of devices. > > https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319 > >> Why was it external and how is it different from your patches? > > As is typical with external trees -- it would seem Vikram actually > wrote the > original request_firmware_into_buf() API for the msm tree. It > contained the > fw_desc changes. Stephen Boyd seems to have worked on the actual > upstreaming > effort and he dropped that fw_desc effort from the upstreaming effort. > > Vikarm noted he had had a similar internal discussion with Stephen > Stephen Boyd > as I am with you on this thread back when request_firmware_into_buf() > was being > upstreamed [0]. He noted that back then reason for this proposed change > was > that "the number of things being passed around between layers of > functions > inside firmware_class seemed a bit untenable". I will note around that > time I > had proposed a similar change using the fw_desc name, it was only later > that > this renamed to a params postfix as Linus did not like the descriptor > name. > > [0] > https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org > > The only difference is that his patch does only modifying the private > members > of the internal API and routines from my patch 1/5, and he kept the > "descriptor" name Linus disliked a while ago. This is precisely why > AKASHI > noted I could split up my patch 1 in more ways in this series to help > *patch > review*. > >> Was it used because your version has taken so long to be >> submitted/reviwed? > > Vikram would have a better idea as he is the one who authored it, but > it would > seem this effort was in parallel to my own at that time. > I must shamefully admit that the story is a bit older - the patch I originally worked on was on a v3.4 based tree. We had been forward porting it until Stephen Boyd was kind enough (or tired of it) to take time out of his clock maintainer-ship and upstream the request_firmware_into_buf API. At that point of time it seemed that the 'desc' approach was unnecessary, and I agreed. So Luis's series came in much later and wasn't a factor in forward-porting the patches. While it does seem that the _internal_ implementation of firmware_class can be a bit friendlier to adding the features that are on their way, I can't say the same about the API being exposed to drivers in mainline; maintainers and folks with more experience in kernel API evolution are better equipped to answer that question. Thanks, Vikram