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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Jun 2017 09:27:45 -0700
From:   Vikram Mulukutla <markivx@...eaurora.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, wagi@...om.org,
        dwmw2@...radead.org, rafal@...ecki.pl,
        arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        yi1.li@...ux.intel.com, atull@...nsource.altera.com,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org, luto@...nel.org,
        torvalds@...ux-foundation.org, keescook@...omium.org,
        takahiro.akashi@...aro.org, dhowells@...hat.com, pjones@...hat.com,
        hdegoede@...hat.com, alan@...ux.intel.com, tytso@....edu,
        linux-kernel@...r.kernel.org, linux-kernel-owner@...r.kernel.org
Subject: Re: [PATCH v9 1/5] firmware: add extensible driver data params


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. 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]. 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. 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.

     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, 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)?

Thanks,
Vikram

[1] - 
https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319