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]

Message-ID: <20170621004934.GH4820@linaro.org>
Date:   Wed, 21 Jun 2017 09:49:35 +0900
From:   AKASHI Takahiro <takahiro.akashi@...aro.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Vikram Mulukutla <markivx@...eaurora.org>,
        Greg KH <gregkh@...uxfoundation.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,
        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

On Tue, Jun 20, 2017 at 07:22:19PM +0200, Luis R. Rodriguez wrote:
> On Tue, Jun 20, 2017 at 09:27:45AM -0700, Vikram Mulukutla wrote:
> > 
> > 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.

Given the discussions here, it would be better to split your [1/5] and
[2/5] into more smaller pieces, say,
  * re-factor the old APIs with introducing private fw_desc
  * add new APIs with extra public arguments
  * extend new APIs per-feature
      - sync/async callbacks
      - API version, and so on

This way, you can illustrate how your approach evolves and it may
mitigate people's concern about how complicated it is on the surface,
allowing for discussing each of aspects of new APIs separately.

Thanks,
-Takahiro AKASHI

>   Luis