Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751040AbdFTRWY (ORCPT ); Tue, 20 Jun 2017 13:22:24 -0400 Received: from mx2.suse.de ([195.135.220.15]:41104 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750816AbdFTRWX (ORCPT ); Tue, 20 Jun 2017 13:22:23 -0400 Date: Tue, 20 Jun 2017 19:22:19 +0200 From: "Luis R. Rodriguez" To: Vikram Mulukutla Cc: Greg KH , "Luis R. Rodriguez" , wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl, arend.vanspriel@broadcom.com, rjw@rjwysocki.net, yi1.li@linux.intel.com, 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, linux-kernel-owner@vger.kernel.org Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params Message-ID: <20170620172219.GQ21846@wotan.suse.de> 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> <20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5087 Lines: 112 On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote: > > Hi Greg, Luis, > > On 2017-06-17 12:38, 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. > > > > > If not, what concrete alternatives do you suggest? > > > > It's working, so leave it alone? :) > > > > So I wanted to note here that I had a similar internal discussion with > Stephen Boyd when he first upstreamed the request_firmware_into_buf API. Thanks for sharing! > I actually did change things a bit to pass around a 'desc' structure with all > the flags and parameters in our internal vendor tree [1]. I'll note the "desc" approach and name was what I originally used also on my earlier incarnations of the driver data API (back then the "sysdata API"). > This is a sort of > an ultra-lite version of what Luis is trying to do - extensibility - but does > not change the API for driver owners. What I propose does not change the API for existing drivers either. The changes I propose allows the flexibility both internally and externally but only externally for newer APIs and features. > Existing APIs become wrappers around > the new API. My primary reason then was the number of things being passed > around between layers of functions inside firmware_class seemed a bit > untenable. Yup!! Same applies to the public API! > Just like Greg, Stephen also brought up the argument about why the > unnecessary complication to the API without any measurable benefit - this > seemed a fair argument to me at that time. But here's my point - if Luis > agrees that _this_ patchset is going slightly Mjolnir, One of the issues you'd face with your changes is you are essentially diverging now with *similar* cleanup but its *not upstream*, so fixes may require manual manipulation! The place to address what you need is not a private vendor tree but *upstream* otherwise your tree will diverge and can make it hard for you to merge stable fixes or cherry pick enhancements. The fact that folks are willing to go the such lengths as to *merge* side internal cleanups to internal APIs should say a lot of confirming the need for my own work. I stand by this work as needed, I have been the one doing the heavy lifting of addressing fixes, documenting it and looking at a real sensible way to extend it, as such I really do believe its needed. I'm tired of seeing side hacks and people throwing gum at things. > perhaps a light > internal rework inside firmware_class might be more suitable towards the > extensibility goal while keeping driver API usage as simple as it is today > (even if you hate my patch for various reasons)? > > [1] - https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319 What you did is but one step I took, your changes make it easier to shuffle data around internally. Its not sufficient to clean things up well enough, for instance look at the "firmware behavior options" which are cleaned up in this patch 1/5. That's been one pain on our side for a while, people understanding when a flag applies or a feature, and making sure we document it all. Then, one of the end goals is to provide extensibility, this is to allow users to *pass* similar type of struct for either a sync or async call. Without this the API remains unflexible and I predict we'll end up with the same situation later anyway. The approach I took uses an internal struct for passing required data for the private internal API use. Then it also provides a public struct which can be used to grow requirements to make only *new* API easily extensible. So we need all three things to move forward. Luis