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

Groups > linux.kernel > #1673827 > 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 Greg KH <gregkh@linuxfoundation.org> - 2017-06-23 22:30 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-24 00:50 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-24 01:10 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-24 02:50 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-24 14:50 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 19:40 +0200
              Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-26 22:50 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 23:40 +0200
              Re: [PATCH v9 1/5] firmware: add extensible driver data params Vikram Mulukutla <markivx@codeaurora.org> - 2017-06-27 04:30 +0200
                Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 19:30 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-24 14:50 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 18:00 +0200

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

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.

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

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.

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

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.

thanks,

greg k-h

[toc] | [next] | [standalone]

#1673931

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

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

#1673938

On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>
> 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*.

This has been going on forever. Everybody hates your data-driven one.
It's too indirect, it adds all those nasty "descriptors" of what to
do, and it doesn't match what the current model does at all.

Things like that may be ok as an internal implementation, but even
there it's questionable if it then means a big disconnect between what
people actually use (the normal functional model) and the
implementation.

The thing is, it's much better to just have functions that load the
firmware data. Have them named that way ("load_firmware()"), and act
that way ("just load the damn firmware file") instead of having odd
descriptors that describe what is goign to be done and some state for
it, and then get passed around.

Don't add this kind of crazy abstraction complexity.

If somebody wants to veryify a signature on a firmware file, they
should *NOT* fill in a descriptor that says "check signature when
loading". Thats' complete BS.

They should just do "load_firmware()" and then "check_signature()" or whatever.

Would such a "load firmware file, then check signature" take a few
lines (with error handling)? Yes.

But it is a *simple* interface. It doesn't have some stupid "struct
driver_data_params" that needs to be filled in with random details (or
has magic macros in a header file that fill in default values). It's
_straightforward_.

                Linus

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

#1673962

On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote:
> On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >
> > 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*.
> 
> This has been going on forever. Everybody hates your data-driven one.

Before you, the only person who had expressed disdain here was Greg.

> It's too indirect, it adds all those nasty "descriptors" of what to
> do, and it doesn't match what the current model does at all.

This is good feedback, I do accept deciding where to draw the line is hard.
I decided to go with blocking/non-blocking as the fine line.

> Things like that may be ok as an internal implementation, but even
> there it's questionable if it then means a big disconnect between what
> people actually use (the normal functional model) and the
> implementation.

A vendor tree implemented their *own* solution and were willing to maintain
it despite this likely making it hard to port stable fixes. That I think says
a lot for a need...

> The thing is, it's much better to just have functions that load the
> firmware data. Have them named that way ("load_firmware()"), and act
> that way ("just load the damn firmware file") instead of having odd
> descriptors that describe what is goign to be done and some state for
> it, and then get passed around.
> 
> Don't add this kind of crazy abstraction complexity.
> 
> If somebody wants to veryify a signature on a firmware file, they
> should *NOT* fill in a descriptor that says "check signature when
> loading". Thats' complete BS.
> 
> They should just do "load_firmware()" and then "check_signature()" or whatever.

That's a fair suggestion for firmware signing! And I'll let AKASHI comment on
whether or not that would suffice for his requirements given he's now
addressing firmware signing.

There are still other requirements and features in the pipeline for which we
can consider parameters to parse for, rather than adding new API. Case in
point, do we want *one* API just to disable the firmware cache? Specially
knowing that another feature in the pipeline later would make use of this as a
requirement?

Or let us just consider the very simple *optional* async firmware. Do we add
*one* full new API call just for that?

  Luis

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

#1674077

On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote:
> On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote:
> > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > >
> > > 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*.
> > 
> > This has been going on forever. Everybody hates your data-driven one.
> 
> Before you, the only person who had expressed disdain here was Greg.

Very few people actually review code, you know that.

> > Things like that may be ok as an internal implementation, but even
> > there it's questionable if it then means a big disconnect between what
> > people actually use (the normal functional model) and the
> > implementation.
> 
> A vendor tree implemented their *own* solution and were willing to maintain
> it despite this likely making it hard to port stable fixes. That I think says
> a lot for a need...

What vendor tree?  Where was it shipped?  Why was it external and how is
it different from your patches?  Was it used because your version has
taken so long to be submitted/reviwed?

> There are still other requirements and features in the pipeline for which we
> can consider parameters to parse for, rather than adding new API. Case in
> point, do we want *one* API just to disable the firmware cache? Specially
> knowing that another feature in the pipeline later would make use of this as a
> requirement?

Again, I do not care!  You can not justify patches today with some
mythical thing in the future that might never even happen.

Again, as it stands, this patch series is unacceptable, and the added
complexity of a crazy api that goes against almost all normal in-kernel
apis, is only one part of the reason.  The other being the loads of
added code for no apparent benifit at all.

So please both fix the api to be "normal", and show as to why these
patches are actually needed _today_, otherwise we can just live with
what we have now just fine and muddle along like always.

thanks,

greg k-h

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

#1674966

On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote:
> On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote:
> > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote:
> > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > > >
> > > > 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*.
> > > 
> > > This has been going on forever. Everybody hates your data-driven one.
> > 
> > Before you, the only person who had expressed disdain here was Greg.
> 
> Very few people actually review code, you know that.

Using that logic, then of course "everybody" was *very* fitting ;)

Then again others who actually are working on extending the firmware API (Yi
Li), or maintaining vendor trees (Vikram), did express their opinions on the
current codebase and their appreciate for the changes I made, however this went
selectively unnoticed.

> > > Things like that may be ok as an internal implementation, but even
> > > there it's questionable if it then means a big disconnect between what
> > > people actually use (the normal functional model) and the
> > > implementation.
> > 
> > A vendor tree implemented their *own* solution and were willing to maintain
> > it despite this likely making it hard to port stable fixes. That I think says
> > a lot for a need...
> 
> What vendor tree?  Where was it shipped? 

The msm-3.18 kernel [0], so assuming this goes to mobile devices, this could
mean millions of devices.

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

> Why was it external and how is it different from your patches?

As is typical with external trees -- it would seem Vikram actually wrote the
original request_firmware_into_buf() API for the msm tree.  It contained the
fw_desc changes. Stephen Boyd seems to have worked on the actual upstreaming
effort and he dropped that fw_desc effort from the upstreaming effort.

Vikarm noted he had had a similar internal discussion with Stephen Stephen Boyd
as I am with you on this thread back when request_firmware_into_buf() was being
upstreamed [0]. He noted that back then reason for this proposed change was
that "the number of things being passed around between layers of functions
inside firmware_class seemed a bit untenable". I will note around that time I
had proposed a similar change using the fw_desc name, it was only later that
this renamed to a params postfix as Linus did not like the descriptor name.

[0] https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org

The only difference is that his patch does only modifying the private members
of the internal API and routines from my patch 1/5, and he kept the
"descriptor" name Linus disliked a while ago. This is precisely why AKASHI
noted I could split up my patch 1 in more ways in this series to help *patch
review*.

> Was it used because your version has taken so long to be submitted/reviwed?

Vikram would have a better idea as he is the one who authored it, but it would
seem this effort was in parallel to my own at that time.

> > There are still other requirements and features in the pipeline for which we
> > can consider parameters to parse for, rather than adding new API. Case in
> > point, do we want *one* API just to disable the firmware cache? Specially
> > knowing that another feature in the pipeline later would make use of this as a
> > requirement?
> 
> Again, I do not care!  You can not justify patches today with some
> mythical thing in the future that might never even happen.

Some of these features are things actually being discussed for a while, so to
say they are mythical is not accurate. I can trace back firmware signing
discussions back to 2015, along with Plumbers in person discussions where we
seem to have agreed upon a path forward among a few folks who disagreed on a
technical basis. Linaro has a clear interest so AKASHI picked up that work now
as I have been busy with general maintainer duties. The fact that Linus just
suggested an alternative approach to a params approach is new, and yet to be
reviewe by AKASHI for firmware signing.

Granting the option to make async firmware optional was discussed since
December 2016 by RafaÅ [1]. It was only later during my driver data API changes
that Hans noted the nvram part was actually *not* optional [2] so this
requirement dropped. *However* as the maintainer I believ ethis requirement *is
sensible* and would not be surprised if alternative firmware already exists
where this is what is intended. 

The streaming support for FPGAs has been going through a round of reviews since
March [3]. The fact that you only become aware or jump into review now does not
make them mythical.

[1] https://lkml.kernel.org/r/CACna6rxOGo0e9U7eXpUgnnBuxL+x1B0JBf9ZBq2WPbaBE=YZ-g@mail.gmail.com
[2] https://lkml.kernel.org/r/09063fc2-af77-ced6-ed90-ab20e2884969@redhat.com
[3] https://lkml.kernel.org/r/1489105090-4996-1-git-send-email-yi1.li@linux.intel.com

> Again, as it stands, this patch series is unacceptable, and the added
> complexity of a crazy api that goes against almost all normal in-kernel
> apis, is only one part of the reason.

Back to the *real* highlight and *crux* of this thread:

This a paradigm position, and I'm fine to go with it! I sure hope my original
logic here is not forgotten, the goal was to demo and show an API which
mitigates unnecessary collateral evolutions. I'll also note an *alternative* has
not yet been clearly suggested, so I'd be *delighted* to hear alternatives.

> The other being the loads of added code for no apparent benifit at all.

I can split up patches as AKASHI suggested.

> So please both fix the api to be "normal",

Will do, but an alternative to the approach would be appreciated. Take the
optional firmware for async requests as a good example to start with.

> and show as to why these
> patches are actually needed _today_, otherwise we can just live with
> what we have now just fine and muddle along like always.

I think a cleanup for the internal API, flags, and things shuffling back and
forward makes sense already so will start off with *just that*.

  Luis

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

#1675093

On 2017-06-26 19:33, Luis R. Rodriguez wrote:
> On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote:
>> On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote:
>> > On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote:
>> > > On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > > >
>> > > > 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*.
>> > >
>> > > This has been going on forever. Everybody hates your data-driven one.
>> >
>> > Before you, the only person who had expressed disdain here was Greg.
>> 
>> Very few people actually review code, you know that.
> 
> Using that logic, then of course "everybody" was *very* fitting ;)
> 
> Then again others who actually are working on extending the firmware 
> API (Yi
> Li), or maintaining vendor trees (Vikram), did express their opinions 
> on the
> current codebase and their appreciate for the changes I made, however 
> this went
> selectively unnoticed.
> 
>> > > Things like that may be ok as an internal implementation, but even
>> > > there it's questionable if it then means a big disconnect between what
>> > > people actually use (the normal functional model) and the
>> > > implementation.
>> >
>> > A vendor tree implemented their *own* solution and were willing to maintain
>> > it despite this likely making it hard to port stable fixes. That I think says
>> > a lot for a need...
>> 
>> What vendor tree?  Where was it shipped?
> 
> The msm-3.18 kernel [0], so assuming this goes to mobile devices, this 
> could
> mean millions of devices.
> 
> https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319
> 
>> Why was it external and how is it different from your patches?
> 
> As is typical with external trees -- it would seem Vikram actually 
> wrote the
> original request_firmware_into_buf() API for the msm tree.  It 
> contained the
> fw_desc changes. Stephen Boyd seems to have worked on the actual 
> upstreaming
> effort and he dropped that fw_desc effort from the upstreaming effort.
> 
> Vikarm noted he had had a similar internal discussion with Stephen 
> Stephen Boyd
> as I am with you on this thread back when request_firmware_into_buf() 
> was being
> upstreamed [0]. He noted that back then reason for this proposed change 
> was
> that "the number of things being passed around between layers of 
> functions
> inside firmware_class seemed a bit untenable". I will note around that 
> time I
> had proposed a similar change using the fw_desc name, it was only later 
> that
> this renamed to a params postfix as Linus did not like the descriptor 
> name.
> 
> [0] 
> https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org
> 
> The only difference is that his patch does only modifying the private 
> members
> of the internal API and routines from my patch 1/5, and he kept the
> "descriptor" name Linus disliked a while ago. This is precisely why 
> AKASHI
> noted I could split up my patch 1 in more ways in this series to help 
> *patch
> review*.
> 
>> Was it used because your version has taken so long to be 
>> submitted/reviwed?
> 
> Vikram would have a better idea as he is the one who authored it, but 
> it would
> seem this effort was in parallel to my own at that time.
> 
>> > There are still other requirements and features in the pipeline for which we
>> > can consider parameters to parse for, rather than adding new API. Case in
>> > point, do we want *one* API just to disable the firmware cache? Specially
>> > knowing that another feature in the pipeline later would make use of this as a
>> > requirement?
>> 
>> Again, I do not care!  You can not justify patches today with some
>> mythical thing in the future that might never even happen.
> 
> Some of these features are things actually being discussed for a while, 
> so to
> say they are mythical is not accurate. I can trace back firmware 
> signing
> discussions back to 2015, along with Plumbers in person discussions 
> where we
> seem to have agreed upon a path forward among a few folks who disagreed 
> on a
> technical basis. Linaro has a clear interest so AKASHI picked up that 
> work now
> as I have been busy with general maintainer duties. The fact that Linus 
> just
> suggested an alternative approach to a params approach is new, and yet 
> to be
> reviewe by AKASHI for firmware signing.
> 
> Granting the option to make async firmware optional was discussed since
> December 2016 by RafaÅ [1]. It was only later during my driver data API 
> changes
> that Hans noted the nvram part was actually *not* optional [2] so this
> requirement dropped. *However* as the maintainer I believ ethis 
> requirement *is
> sensible* and would not be surprised if alternative firmware already 
> exists
> where this is what is intended.

I believe there was a misunderstanding of my patch by Hans. The point of 
my
patch was to don't display warning *IF* we can use alternative soruce 
and
get the NVRAM (firmware) from platform data (special partition used by 
the
bootloader and accessible by the operating system).

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

#1675117

On Mon, Jun 26, 2017 at 08:19:07PM +0200, Rafał Miłecki wrote:
> On 2017-06-26 19:33, Luis R. Rodriguez wrote:
> > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote:
> > > > There are still other requirements and features in the pipeline for which we
> > > > can consider parameters to parse for, rather than adding new API. Case in
> > > > point, do we want *one* API just to disable the firmware cache? Specially
> > > > knowing that another feature in the pipeline later would make use of this as a
> > > > requirement?
> > > 
> > > Again, I do not care!  You can not justify patches today with some
> > > mythical thing in the future that might never even happen.
> > 
> > Granting the option to make async firmware optional was discussed since
> > December 2016 by RafaÅ [1]. It was only later during my driver data API
> > changes that Hans noted the nvram part was actually *not* optional [2] so
> > this requirement dropped. *However* as the maintainer I believ ethis
> > requirement *is sensible* and would not be surprised if alternative
> > firmware already exists where this is what is intended.
> 
> I believe there was a misunderstanding of my patch by Hans. The point of my
> patch was to don't display warning *IF* we can use alternative soruce and
> get the NVRAM (firmware) from platform data (special partition used by the
> bootloader and accessible by the operating system).

Oh, are you saying the optional async firmware loading is still a requirement
for this driver? Are you, Hans, and Arend Van Spriel in agreement on this?

If so then that definitely makes 3 effective changes in my radar for extensions
to the firmware API.

  Luis

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

#1675255

On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote:
> On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote:
>> On Sat, Jun 24, 2017 at 02:48:28AM +0200, Luis R. Rodriguez wrote:
>>> On Fri, Jun 23, 2017 at 04:09:29PM -0700, Linus Torvalds wrote:
>>>> On Fri, Jun 23, 2017 at 3:43 PM, Luis R. Rodriguez 
>>>> <mcgrof@kernel.org> wrote:
>>>>> 
>>>>> 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*.
>>>> 
>>>> This has been going on forever. Everybody hates your data-driven 
>>>> one.
>>> 
>>> Before you, the only person who had expressed disdain here was Greg.
>> 
>> Very few people actually review code, you know that.
> 
> Using that logic, then of course "everybody" was *very* fitting ;)
> 
> Then again others who actually are working on extending the firmware 
> API (Yi
> Li), or maintaining vendor trees (Vikram), did express their opinions 
> on the
> current codebase and their appreciate for the changes I made, however 
> this went
> selectively unnoticed.
> 
>>>> Things like that may be ok as an internal implementation, but even
>>>> there it's questionable if it then means a big disconnect between 
>>>> what
>>>> people actually use (the normal functional model) and the
>>>> implementation.
>>> 
>>> A vendor tree implemented their *own* solution and were willing to 
>>> maintain
>>> it despite this likely making it hard to port stable fixes. That I 
>>> think says
>>> a lot for a need...
>> 
>> What vendor tree?  Where was it shipped?
> 
> The msm-3.18 kernel [0], so assuming this goes to mobile devices, this 
> could
> mean millions of devices.
> 
> https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=msm-3.18&id=7aa7efd3c150840369739893a84bd1d9f9774319
> 
>> Why was it external and how is it different from your patches?
> 
> As is typical with external trees -- it would seem Vikram actually 
> wrote the
> original request_firmware_into_buf() API for the msm tree.  It 
> contained the
> fw_desc changes. Stephen Boyd seems to have worked on the actual 
> upstreaming
> effort and he dropped that fw_desc effort from the upstreaming effort.
> 
> Vikarm noted he had had a similar internal discussion with Stephen 
> Stephen Boyd
> as I am with you on this thread back when request_firmware_into_buf() 
> was being
> upstreamed [0]. He noted that back then reason for this proposed change 
> was
> that "the number of things being passed around between layers of 
> functions
> inside firmware_class seemed a bit untenable". I will note around that 
> time I
> had proposed a similar change using the fw_desc name, it was only later 
> that
> this renamed to a params postfix as Linus did not like the descriptor 
> name.
> 
> [0] 
> https://lkml.kernel.org/r/20ac6fa65c8ff4ef83386aa1e8d5ca91@codeaurora.org
> 
> The only difference is that his patch does only modifying the private 
> members
> of the internal API and routines from my patch 1/5, and he kept the
> "descriptor" name Linus disliked a while ago. This is precisely why 
> AKASHI
> noted I could split up my patch 1 in more ways in this series to help 
> *patch
> review*.
> 
>> Was it used because your version has taken so long to be 
>> submitted/reviwed?
> 
> Vikram would have a better idea as he is the one who authored it, but 
> it would
> seem this effort was in parallel to my own at that time.
> 

I must shamefully admit that the story is a bit older - the patch I
originally worked on was on a v3.4 based tree. We had been forward
porting it until Stephen Boyd was kind enough (or tired of it) to take
time out of his clock maintainer-ship and upstream the
request_firmware_into_buf API. At that point of time it seemed that the
'desc' approach was unnecessary, and I agreed. So Luis's series came
in much later and wasn't a factor in forward-porting the patches.
     While it does seem that the _internal_ implementation of
firmware_class can be a bit friendlier to adding the features that
are on their way, I can't say the same about the API being exposed to
drivers in mainline; maintainers and folks with more experience in
kernel API evolution are better equipped to answer that question.

Thanks,
Vikram

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

#1676019

On Mon, Jun 26, 2017 at 07:28:12PM -0700, Vikram Mulukutla wrote:
> On 6/26/2017 10:33 AM, Luis R. Rodriguez wrote:
> > On Sat, Jun 24, 2017 at 02:39:51PM +0200, Greg KH wrote:
> > > Was it used because your version has taken so long to be
> > > submitted/reviwed?
> > 
> > Vikram would have a better idea as he is the one who authored it, but it
> > would seem this effort was in parallel to my own at that time.
> > 
> 
> I must shamefully admit that the story is a bit older - the patch I
> originally worked on was on a v3.4 based tree. 
 
Oh wow so we had *two* separate parallel efforts to simplify this code
somehow...

My earliest sysdata API was based on v4.2-rc5 [0], this was after we decided we
*wanted* to enable to pass more arguments for fw signing from the start, to
enable custom fw criteria, as my original fw signing effort was completely
transparent to the API and matched what we did with module signing [1], and
based on v4.1-rc3.

Only difference is you just worked on the internal data tossed around. I
provided a way to also use this for growing the API.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20150805-sysdata
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=fw-signing-v2-20150513

> We had been forward
> porting it until Stephen Boyd was kind enough (or tired of it) to take
> time out of his clock maintainer-ship and upstream the
> request_firmware_into_buf API. At that point of time it seemed that the
> 'desc' approach was unnecessary, and I agreed.

It was very much needed and it could have helped. Next time please just send
patches right away!

> So Luis's series came
> in much later and wasn't a factor in forward-porting the patches.
>     While it does seem that the _internal_ implementation of
> firmware_class can be a bit friendlier to adding the features that
> are on their way, I can't say the same about the API being exposed to
> drivers in mainline; maintainers and folks with more experience in
> kernel API evolution are better equipped to answer that question.

I actually am not aware how seriously the postulation to the problem I decided
to take on is being considered here...

Some of it may seem straight forward to some based on experience, but due to
the size of the kernel inspired by my prior effort to study collateral
evolutions for both forward and backporting purposes, I've decided to take on
the problem in a bit different light.

Just as your primary reason for your changes was that "the number of things
being passed around between layers of functions inside firmware_class seemed a
bit untenable", I also believed that the way in which we were loosely growing
the firmware API through unnecessary collateral evolutions was untenable.

I will confess that growing the API was just one consideration, another long
term lofty goal also aims towards automatic test driver generation, and enough
is sprinkled on test_driver_data.c that I hope some could infer perhaps how
I started thinking that might be possible one day.

I'm happy to park such effort, so long as we just decide with a path forward so
we can move on and chug on. Perhaps in the future some folks may want to
re-evaluate and consider the approach a bit further.

  Luis

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

#1674078

On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote:
> > 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!

Ok, so we are all caught up now with bug fixes?  That's good, I was not
aware of that, so nevermind that objection, my fault.

greg k-h

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

#1674900

On Sat, Jun 24, 2017 at 02:40:57PM +0200, Greg KH wrote:
> On Sat, Jun 24, 2017 at 12:43:38AM +0200, Luis R. Rodriguez wrote:
> > > 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!
> 
> Ok, so we are all caught up now with bug fixes?  That's good, I was not
> aware of that, so nevermind that objection, my fault.

Yes. I've been following up on all actively reported bugs and have even
implemented better alternatives based on discussions.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web