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

[Luis R. Rodriguez]

On Fri, Jun 23, 2017 at 11:51:23PM +0800, Greg KH wrote:
> On Mon, Jun 19, 2017 at 09:35:22PM +0200, Luis R. Rodriguez wrote:
> > > 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.
> > 
> > There are two things to consider:
> > 
> > a) The current design of the firmware API, and interfaces with exported symbols
> > 
> > The case for the driver data API was that we were being super sloppy with extensions,
> > to the point was making the internal code base very bug prone and full or redirect
> > conditionals with #ifdefery nightmware stuff.
> >    
> > b) Features of the firmware API
> > 
> > These have to be evaluated on a case by case basis.
> 
> Wait, no, you didn't address my main complaint at all here.  You are
> adding complexity for no perceived gain at all with this patch set.
> 
> Now you might feel that this series gets you moving forward toward an
> end goal of reduced complexity and wonderfulness, but you know how
> kernel development works, you have to justify _all_ of your changes, not
> just some future end result that is not even presented here.

My point was that a) was already complex, so to say what I'm adding
to address a) is complex would be unfair, so rather the question should
be if its less complex, or if there are valid technical issues I'd like
to hear them.

> <wall of text snipped>
> 
> I, and others I know, have told you to work on simplifying your
> responses, and descriptions, of patches.  Take the extra time to make a
> shorter answer.  You will get better results, as I dread having to read
> and respond to them currently.

Sure, point taken!

> I know you have spent a lot of time and effort on this work, but as it
> stands, this crazy new interface (data-driven api vs. the traditional
> procedural apis we know and love in Linux), is not acceptable at all.

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*. If by data-driven you mean using structs to drive the
requirements, instead of adding more API per new feature.

I would strongly disagree that we always prefer adding new functional APIs
loosely.  I think this should be reviewed on a case by case basis and it should
be up to the maintainer who has better visibility into the history of the code,
and what is coming. A quick git log grep found a recent commit example where we
take on a flexible API on other *random* places in Linux. Refer to for example
the change form bioset_create_nobvec() to bioset_create() and use flags in the
patch titled "blk: replace bioset_create_nobvec() with a flags arg to
bioset_create()", present on linux-next [0] followed by "blk: make the bioset
rescue_workqueue optional." [1] which also extends the flags and uses them.

To argue that we *still* need to keep doing a functional approach for the
firmware API and keep adding new routines for new features, seems insane to me
at this point -- if that is what you were suggesting...

Also, the reason *why* I think this discussion is important is that it also
implicates the amount of collateral evolutions needed per API. If you embrace
data-driven API (or flags, or structs for APIs) you should see less patches
due to collateral evolutions. In a way its my own resolution to mitigate
*unnecessary* collateral evolutions by proper architecture. Where that line is
drawn should be up to the designers of the API.

For system calls, for instance, I think its well accepted we *never* want to
make inflexible APIs. There is strong history for why that is the case. We
cannot compare system calls to exported symbols, at all, but we can learn a bit
from the flexibility efforts on them for exported symbols as well.  Its *real
news* to me that we *always* prefer a procedural preferences for APIs /
exported symbols.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=011067b05668b05aae88e5a24cff0ca0a67ca0b0
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=47e0fb461fca1a68a566c82fcc006cc787312d8c

> It's also blocking real bug fixes and features that people want
> addressed, which isn't acceptable.

What? What bugs ? I have addressed *every single* stable issue in queue and
have sent patches for them and they do not depend in any way on any of this
series! In fact I've taken liberty to go to great lengths to ensure *stable*
proposals *get proper review* so we *do the right thing*. Case in point was the
recent -ERESTART crap which in the end we ended up moving towards preferring
adding new swait API for stable for it.

I have ZERO stable fixes pending on my queue! Please name the bug pending.

In so far as features.. yes ! This delays adding new features because the crux
of the discussion is *how* to add them, a flag or new parameter in a struct
*or* do we add yet-another-API-call? I'm making a case for the first.

> Please take the time to step back, and see if you really want to spend
> the effort into creating something that you can easily justify and break
> down into acceptable patches.  If so, great, do it, but as it stands
> today, that is not what you have done here, at all.

AKASHI addressed how to break down the patches further. I will take that on!
Your point above on data-driven Vs functional however still deserves some
discussion as otherwise the effort on the changes I've made are pointless.

  Luis