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

[Luis R. Rodriguez]

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