Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670929 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v9 1/5] firmware: add extensible driver data params Vikram Mulukutla <markivx@codeaurora.org> - 2017-06-20 18:30 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-20 19:30 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-21 03:00 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-23 18:40 +0200

#1670929 — 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

[toc] | [next] | [standalone]

#1670971

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

[toc] | [prev] | [next] | [standalone]

#1671263

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

[toc] | [prev] | [next] | [standalone]

#1673673

On Wed, Jun 21, 2017 at 09:49:35AM +0900, AKASHI Takahiro wrote:
> 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

So patch 1/5 introduces 3 structs:

o struct driver_data_req_params  - used for user specified parameters
o struct driver_data_priv_params - used for internal use only
o struct driver_data_params - stiches both of the above together,
  only for internal use

I could certainly split the patch to introduce each separately.

>   * add new APIs with extra public arguments

This was split out already, patch 2 is the first patch introducing new API.

>   * extend new APIs per-feature
>       - sync/async callbacks

I suppose the base would be what we have today, only in new form. And sure,
I can add the features one by one...

>       - API version, and so on

Right.

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

This makes sense. Greg, does this make sense to you? Or are you stone cold
against all this?

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web