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

Groups > linux.kernel > #1664617 > 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-13 11:10 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-13 13:10 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-13 15:20 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params Rafał Miłecki <rafal@milecki.pl> - 2017-06-13 16:50 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-13 17:40 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-13 18:00 +0200
    Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-13 21:50 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-14 18:00 +0200
      Re: [PATCH v9 1/5] firmware: add extensible driver data params Greg KH <gregkh@linuxfoundation.org> - 2017-06-18 02:30 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params Johannes Berg <johannes@sipsolutions.net> - 2017-06-19 09:40 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-19 21:50 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-20 03:30 +0200
        Re: [PATCH v9 1/5] firmware: add extensible driver data params "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-19 21:40 +0200
          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
          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 "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-20 01:00 +0200
          Re: [PATCH v9 1/5] firmware: add extensible driver data params AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-06-20 03:50 +0200
            Re: [PATCH v9 1/5] firmware: add extensible driver data params "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-20 17:30 +0200
        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

Page 1 of 2  [1] 2  Next page →

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

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!

:(

Seriously, why?  Why are we extending any of this at all?  This series
adds a ton of new "features" and complexity, but for absolutely no gain.

Oh, I take it back, you removed 29 lines from the iwlwifi driver.

That's still not worth it at all, you have yet to sell me on this whole
complex beast.  I can't see why we need it, and if I, one of the few
people who thinks they actually understand this kernel interface, can't
see it, how can you sell it to someone else?

Sorry, but no, I'm still not going to take this series until you show
some _REAL_ benefit for it.

thanks,

greg k-h

[toc] | [next] | [standalone]

#1664752

On 2017-06-13 11:05, 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!
> 
> :(
> 
> Seriously, why?  Why are we extending any of this at all?  This series
> adds a ton of new "features" and complexity, but for absolutely no 
> gain.
> 
> Oh, I take it back, you removed 29 lines from the iwlwifi driver.
> 
> That's still not worth it at all, you have yet to sell me on this whole
> complex beast.  I can't see why we need it, and if I, one of the few
> people who thinks they actually understand this kernel interface, can't
> see it, how can you sell it to someone else?
> 
> Sorry, but no, I'm still not going to take this series until you show
> some _REAL_ benefit for it.

FWIW I saw (or maybe still see?) a need to extend request_firmware* API 
to
allow silencing a warning if firmware file is missing.

I even sent a trivial patch adding support for this:
[PATCH V4 1/2] firmware: add more flexible request_firmware_async 
function
https://patchwork.kernel.org/patch/9588787/
(I think it still applies) but it got rejected due to Luis's big rework.

To be honest after seeing this big & more complex driver data API I just
gave up and decided I don't care about false problem reports that much 
:(

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

#1664839

On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote:
> On 2017-06-13 11:05, 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!
> > 
> > :(
> > 
> > Seriously, why?  Why are we extending any of this at all?  This series
> > adds a ton of new "features" and complexity, but for absolutely no gain.
> > 
> > Oh, I take it back, you removed 29 lines from the iwlwifi driver.
> > 
> > That's still not worth it at all, you have yet to sell me on this whole
> > complex beast.  I can't see why we need it, and if I, one of the few
> > people who thinks they actually understand this kernel interface, can't
> > see it, how can you sell it to someone else?
> > 
> > Sorry, but no, I'm still not going to take this series until you show
> > some _REAL_ benefit for it.
> 
> FWIW I saw (or maybe still see?) a need to extend request_firmware* API to
> allow silencing a warning if firmware file is missing.
> 
> I even sent a trivial patch adding support for this:
> [PATCH V4 1/2] firmware: add more flexible request_firmware_async function
> https://patchwork.kernel.org/patch/9588787/
> (I think it still applies) but it got rejected due to Luis's big rework.

Can you resend this series if it still does apply?

And what exact warning is this silencing?  Normally we want the warning
there, as that implies that something is wrong if the firmware file that
a driver is asking for is not present.  That way the user can know to go
fix it up, right?

thanks,

greg k-h

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

#1664940

On 06/13/2017 03:17 PM, Greg KH wrote:
> On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote:
>> On 2017-06-13 11:05, 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!
>>>
>>> :(
>>>
>>> Seriously, why?  Why are we extending any of this at all?  This series
>>> adds a ton of new "features" and complexity, but for absolutely no gain.
>>>
>>> Oh, I take it back, you removed 29 lines from the iwlwifi driver.
>>>
>>> That's still not worth it at all, you have yet to sell me on this whole
>>> complex beast.  I can't see why we need it, and if I, one of the few
>>> people who thinks they actually understand this kernel interface, can't
>>> see it, how can you sell it to someone else?
>>>
>>> Sorry, but no, I'm still not going to take this series until you show
>>> some _REAL_ benefit for it.
>>
>> FWIW I saw (or maybe still see?) a need to extend request_firmware* API to
>> allow silencing a warning if firmware file is missing.
>>
>> I even sent a trivial patch adding support for this:
>> [PATCH V4 1/2] firmware: add more flexible request_firmware_async function
>> https://patchwork.kernel.org/patch/9588787/
>> (I think it still applies) but it got rejected due to Luis's big rework.
>
> Can you resend this series if it still does apply?

Sure, if you think it's worth trying, I'll do that!


> And what exact warning is this silencing?  Normally we want the warning
> there, as that implies that something is wrong if the firmware file that
> a driver is asking for is not present.  That way the user can know to go
> fix it up, right?

It's because brcmfmac looks for NVRAM in two places: /lib/firmware/ and
platform NVRAM. It's supposed to silence
[   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
in case there is platform NVRAM present.

For more details please take a look at:
[PATCH V4 2/2] brcmfmac: don't warn user about NVRAM if fallback to platform one succeeds
https://patchwork.kernel.org/patch/9588791/

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

#1664963

On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote:
> On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote:
> > On 2017-06-13 11:05, 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!
> > > 
> > > :(
> > > 
> > > Seriously, why?  Why are we extending any of this at all?  This series
> > > adds a ton of new "features" and complexity, but for absolutely no gain.
> > > 
> > > Oh, I take it back, you removed 29 lines from the iwlwifi driver.
> > > 
> > > That's still not worth it at all, you have yet to sell me on this whole
> > > complex beast.  I can't see why we need it, and if I, one of the few
> > > people who thinks they actually understand this kernel interface, can't
> > > see it, how can you sell it to someone else?
> > > 
> > > Sorry, but no, I'm still not going to take this series until you show
> > > some _REAL_ benefit for it.
> > 
> > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to
> > allow silencing a warning if firmware file is missing.
> > 
> > I even sent a trivial patch adding support for this:
> > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function
> > https://patchwork.kernel.org/patch/9588787/
> > (I think it still applies) but it got rejected due to Luis's big rework.
> 
> Can you resend this series if it still does apply?

FWIW just some notes on Rafał's series:

someone else brought up second that his second no longer should be applied as
some devices do need what seems to be today's optional request. Also note that
the approach follows the same I take, just struct a firmware_opts instead of
driver params... and it does not mesh up the old options as I did in my first
patch in this series.

  Luis

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

#1664977

On Tue, Jun 13, 2017 at 05:32:49PM +0200, Luis R. Rodriguez wrote:
> On Tue, Jun 13, 2017 at 03:17:43PM +0200, Greg KH wrote:
> > On Tue, Jun 13, 2017 at 12:31:04PM +0200, Rafał Miłecki wrote:
> > > On 2017-06-13 11:05, 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!
> > > > 
> > > > :(
> > > > 
> > > > Seriously, why?  Why are we extending any of this at all?  This series
> > > > adds a ton of new "features" and complexity, but for absolutely no gain.
> > > > 
> > > > Oh, I take it back, you removed 29 lines from the iwlwifi driver.
> > > > 
> > > > That's still not worth it at all, you have yet to sell me on this whole
> > > > complex beast.  I can't see why we need it, and if I, one of the few
> > > > people who thinks they actually understand this kernel interface, can't
> > > > see it, how can you sell it to someone else?
> > > > 
> > > > Sorry, but no, I'm still not going to take this series until you show
> > > > some _REAL_ benefit for it.
> > > 
> > > FWIW I saw (or maybe still see?) a need to extend request_firmware* API to
> > > allow silencing a warning if firmware file is missing.
> > > 
> > > I even sent a trivial patch adding support for this:
> > > [PATCH V4 1/2] firmware: add more flexible request_firmware_async function
> > > https://patchwork.kernel.org/patch/9588787/
> > > (I think it still applies) but it got rejected due to Luis's big rework.
> > 
> > Can you resend this series if it still does apply?
> 
> FWIW just some notes on Rafał's series:
> 
> someone else brought up second that his second no longer should be applied as
> some devices do need what seems to be today's optional request. Also note that
> the approach follows the same I take, just struct a firmware_opts instead of
> driver params... and it does not mesh up the old options as I did in my first
> patch in this series.

As I have no idea what his series looks like at the moment, why not wait
until they are posted again to review them? :)

thanks,

greg k-h

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

#1665139

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!

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?

If not, what concrete alternatives do you suggest?

> :(
> 
> Seriously, why?  Why are we extending any of this at all? 

Addressing easy extensibility was a prerequisite of considering firmware signing
support. Its really the *only* reason I started reviewing the firmware API, to the
point I started helping fix quite a bit of bugs and now just became the maintainer.

So my original firmware signing effort, now driven by AKASHI Takahiro, was one
of the original motivations here!

I asked:

	How we should properly extend the API for new functionality in *a real
	clean way* ? This also means cleaning under the rug!

Ease of extensibility is a need we have as otherwise we end up in the same
situation as before, we end up having to add new functionality by extending the
number of arguments to an sync or async call, and as collateral have to changes
*tons* of callers. I think you and I agree on this, correct me if I'm wrong?

Another issue to address when deciding when there is merit for a new API call,
instead of just folding functionality into flags. For this its best consider
not only the future but also what was done in the past given some new desirable
functionality relies on existing mechanisms.

The approach Rafał took does what I do just that it forgets about cleaning
underneath the rug, so I really cannot see how its any different. Case in
point, we have functionality in place today to support no-cache, yet this is a
hidden feature, used implicitly only for one API call, there has been requests
to support the no-cache functionality by Johannes for iwlwifi long ago for the
async call given *they* do their own caching.  How do you suggest do we take
the no-cache functionality used only internally for another API call and apply
it for async?  Do we make a new exported symbol ? Or do we expose this as a
flag on a regular async call?  With the approach I take we expose the internal
functionality clearly and later Li Yi folds it as public API through a
parameter for the async API. Later he extends iwlwifi with just 2 lines of code
to get this functionality. This would later also be used by some other
functionality he's interested in!

Another good example is that the optional call request_firmware_direct() is for
sync, but async also has the same need --  do we add a new API call just for
the same purpose or do we fold some of this functionality as flags?

How do we properly keep evolving functionality for the firmware API? We have
to consider existing features, perhaps some hidden, and new features coming
down the pipe line.

Please take the time to think about all this for a bit, not only the past but
also all the incoming set of features in the pipeline! The FPGA streaming
support for instance reuses the no-cache "internal hidden feature", as well as
the existing request_firmware_into_buf() with some slight variations!  We could
just fold both requirements as parameters.

> This series adds a ton of new "features"

Actually, it *purposely* only folded the optional nature of firmware for async
and added support for enabling drivers to daisy chain requests using a simple
api numbering scheme as a counter to complex internal driver mechanisms --
Intel's solution actually used recursion. I split out this work on its own
series after a long history of working on firmware singing support. I'm now
also cleaning under the rug.

I purposely limited the amount of "features" to get proper review.

> and complexity,

The difficulty to properly understand the firmware code *was there before*, so
I really do believe its unfair for you to say what I have come up with is
complex, question is if its less complex, and *easier* to understand than what
we had before. I'd argue it is easier to understand given 2 developers have
been working off on top of it now and seem to grok it rather well.

Today's code base's complexity is one of the reasons we also we have had quite
a bit of regressions. One of the other issues was lack of proper documentation,
which I have been fixing as of late, but also clearly documenting the purpose
and limitations each new feature. The confusing #ifef'ery over some of the flag
options was also not helpful. This series did away with all that in preference
for annotating we have really 2 modes of operations with a set of features
which can be described in a proper structure, while also driving functionality
on par with functional testing.

> but for absolutely no gain.

I thought I had made the above problem pretty clear before, during some of the
last 9 iteration of the driver data API. Apologies if it was not.

The goal is to enable us to provide an easily extensible API to avoid
unnecessary collateral evolutions. It was only a subset goal of the firmware
signing effort. This in turn also drove me to consider how to clean under the
rug.

Where does one draws the line of a split of an API? After studying the code a
lot I chose that fine line between sync and async. Some even suggested we do
away with this and go with *one* API call, after all an async call really is
nothing more than a sync call with a schedule worker, however knowing easily if
a piece of code blocks or not in a more direct way in the kernel through seemed
important enough for me to draw a clear API distinction.

> Oh, I take it back, you removed 29 lines from the iwlwifi driver.

This I did simply to demo the gain of *one* possible feature.

I could have done something else. For instance, iwlwifi also can use the
no-cache feature. Do we expose a new async call for that alone? There is
also the firmware signing work which tons of people keep asking for.

If I would have bundled all pending features up into one series you would
have told me to break them down. This is why I just demo'd *one* use case.

The rest is in the pipeline, and folks are posting patches based on them.  I
ask you consider all that functionality in light of what we have today and what
that would look like today with our historical approach.

If you look at Yi Li's patchset for the no-cache feature you'd see that when
exposing *old internal functionality* its rather easy and straight forward to
review and understand [0], as another example consider then Yi LI's next
patchset which adds streaming support for FPGAs using the driver data API [1].

So the rest of the "features" you should consider would come with time as with
Yi Li or AKASHI's firmware signing patches [2]. I just added *two* example new
features to demo this. There rest should be considered by getting an idea
of how the API grows and compares to alternatives we have been practicing over
time.

[0] https://lkml.kernel.org/r/1495262819-981-1-git-send-email-yi1.li@linux.intel.com
[1] https://lkml.kernel.org/r/1495262948-1106-4-git-send-email-yi1.li@linux.intel.com
[2] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org

> That's still not worth it at all, you have yet to sell me on this whole
> complex beast.  I can't see why we need it, and if I, one of the few
> people who thinks they actually understand this kernel interface, can't
> see it, how can you sell it to someone else?

Perhaps you have not tried to clean the code and also not cleanly tried to
extend it with all the functionality I have added. I invite you to do so, and
try accomplish the same I have in this series. Naturally I believe some part of
a different outcome it will be due to subjectivity, and if not I'd like to hear
very specific issues with what I propose.

> Sorry, but no, I'm still not going to take this series until you show
> some _REAL_ benefit for it.

I hope what I describe above helps.

  Luis

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

#1666021

On 6/13/2017 2:40 PM, 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!
> 
> 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?
> 
> If not, what concrete alternatives do you suggest?
> 
>> :(
>>
>> Seriously, why?  Why are we extending any of this at all?
> 
> Addressing easy extensibility was a prerequisite of considering firmware signing
> support. Its really the *only* reason I started reviewing the firmware API, to the
> point I started helping fix quite a bit of bugs and now just became the maintainer.
> 
> So my original firmware signing effort, now driven by AKASHI Takahiro, was one
> of the original motivations here!
> 
> I asked:
> 
> 	How we should properly extend the API for new functionality in *a real
> 	clean way* ? This also means cleaning under the rug!
> 
> Ease of extensibility is a need we have as otherwise we end up in the same
> situation as before, we end up having to add new functionality by extending the
> number of arguments to an sync or async call, and as collateral have to changes
> *tons* of callers. I think you and I agree on this, correct me if I'm wrong?
> 
> Another issue to address when deciding when there is merit for a new API call,
> instead of just folding functionality into flags. For this its best consider
> not only the future but also what was done in the past given some new desirable
> functionality relies on existing mechanisms.
> 
> The approach Rafał took does what I do just that it forgets about cleaning
> underneath the rug, so I really cannot see how its any different. Case in
> point, we have functionality in place today to support no-cache, yet this is a
> hidden feature, used implicitly only for one API call, there has been requests
> to support the no-cache functionality by Johannes for iwlwifi long ago for the
> async call given *they* do their own caching.  How do you suggest do we take
> the no-cache functionality used only internally for another API call and apply
> it for async?  Do we make a new exported symbol ? Or do we expose this as a
> flag on a regular async call?  With the approach I take we expose the internal
> functionality clearly and later Li Yi folds it as public API through a
> parameter for the async API. Later he extends iwlwifi with just 2 lines of code
> to get this functionality. This would later also be used by some other
> functionality he's interested in!

Echo, the firmware caching part is indeed complicated. :) Taking example 
of the 10 seconds rule of uncaching, bad things could happen if the end 
drivers could not complete the programming within the time limit or the 
firmware files was removed from filesystem. It might be safer to let end 
drivers to free the cache? Anyway it should only be enabled by those 
drivers need to re-program firmware during suspend/resume instead of as 
an default option, which is expensive for the base firmware class to do 
on each PM cycle.

I do like the concept of expending features as parameters instead of 
adding new API functions.

> 
> Another good example is that the optional call request_firmware_direct() is for
> sync, but async also has the same need --  do we add a new API call just for
> the same purpose or do we fold some of this functionality as flags?
> 
> How do we properly keep evolving functionality for the firmware API? We have
> to consider existing features, perhaps some hidden, and new features coming
> down the pipe line.
> 
> Please take the time to think about all this for a bit, not only the past but
> also all the incoming set of features in the pipeline! The FPGA streaming
> support for instance reuses the no-cache "internal hidden feature", as well as
> the existing request_firmware_into_buf() with some slight variations!  We could
> just fold both requirements as parameters.
> 
>> This series adds a ton of new "features"
> 
> Actually, it *purposely* only folded the optional nature of firmware for async
> and added support for enabling drivers to daisy chain requests using a simple
> api numbering scheme as a counter to complex internal driver mechanisms --
> Intel's solution actually used recursion. I split out this work on its own
> series after a long history of working on firmware singing support. I'm now
> also cleaning under the rug.
> 
> I purposely limited the amount of "features" to get proper review.
> 
>> and complexity,
> 
> The difficulty to properly understand the firmware code *was there before*, so
> I really do believe its unfair for you to say what I have come up with is
> complex, question is if its less complex, and *easier* to understand than what
> we had before. I'd argue it is easier to understand given 2 developers have
> been working off on top of it now and seem to grok it rather well.
> 
> Today's code base's complexity is one of the reasons we also we have had quite
> a bit of regressions. One of the other issues was lack of proper documentation,
> which I have been fixing as of late, but also clearly documenting the purpose
> and limitations each new feature. The confusing #ifef'ery over some of the flag
> options was also not helpful. This series did away with all that in preference
> for annotating we have really 2 modes of operations with a set of features
> which can be described in a proper structure, while also driving functionality
> on par with functional testing.
> 
>> but for absolutely no gain.
> 
> I thought I had made the above problem pretty clear before, during some of the
> last 9 iteration of the driver data API. Apologies if it was not.
> 
> The goal is to enable us to provide an easily extensible API to avoid
> unnecessary collateral evolutions. It was only a subset goal of the firmware
> signing effort. This in turn also drove me to consider how to clean under the
> rug.
> 
> Where does one draws the line of a split of an API? After studying the code a
> lot I chose that fine line between sync and async. Some even suggested we do
> away with this and go with *one* API call, after all an async call really is
> nothing more than a sync call with a schedule worker, however knowing easily if
> a piece of code blocks or not in a more direct way in the kernel through seemed
> important enough for me to draw a clear API distinction.
> 
>> Oh, I take it back, you removed 29 lines from the iwlwifi driver.
> 
> This I did simply to demo the gain of *one* possible feature.
> 
> I could have done something else. For instance, iwlwifi also can use the
> no-cache feature. Do we expose a new async call for that alone? There is
> also the firmware signing work which tons of people keep asking for.
> 
> If I would have bundled all pending features up into one series you would
> have told me to break them down. This is why I just demo'd *one* use case.
> 
> The rest is in the pipeline, and folks are posting patches based on them.  I
> ask you consider all that functionality in light of what we have today and what
> that would look like today with our historical approach.
> 
> If you look at Yi Li's patchset for the no-cache feature you'd see that when
> exposing *old internal functionality* its rather easy and straight forward to
> review and understand [0], as another example consider then Yi LI's next
> patchset which adds streaming support for FPGAs using the driver data API [1].
> 
> So the rest of the "features" you should consider would come with time as with
> Yi Li or AKASHI's firmware signing patches [2]. I just added *two* example new
> features to demo this. There rest should be considered by getting an idea
> of how the API grows and compares to alternatives we have been practicing over
> time.
> 
> [0] https://lkml.kernel.org/r/1495262819-981-1-git-send-email-yi1.li@linux.intel.com
> [1] https://lkml.kernel.org/r/1495262948-1106-4-git-send-email-yi1.li@linux.intel.com
> [2] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org
> 
>> That's still not worth it at all, you have yet to sell me on this whole
>> complex beast.  I can't see why we need it, and if I, one of the few
>> people who thinks they actually understand this kernel interface, can't
>> see it, how can you sell it to someone else?
> 
> Perhaps you have not tried to clean the code and also not cleanly tried to
> extend it with all the functionality I have added. I invite you to do so, and
> try accomplish the same I have in this series. Naturally I believe some part of
> a different outcome it will be due to subjectivity, and if not I'd like to hear
> very specific issues with what I propose.
> 
>> Sorry, but no, I'm still not going to take this series until you show
>> some _REAL_ benefit for it.
> 
> I hope what I describe above helps.
> 
>    Luis
> 

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

#1668493

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?  :)

> > :(
> > 
> > Seriously, why?  Why are we extending any of this at all? 
> 
> Addressing easy extensibility was a prerequisite of considering firmware signing
> support. Its really the *only* reason I started reviewing the firmware API, to the
> point I started helping fix quite a bit of bugs and now just became the maintainer.
> 
> So my original firmware signing effort, now driven by AKASHI Takahiro, was one
> of the original motivations here!

But we don't accept kernel patches for some mythical future option that
might be happening some time in the future.  Heck, I'm still not
convinced that firmware signing isn't anything more than just some
snakeoil in the first place!

So while you mention lots of times that all sorts of wonderful things
can now possibly be built on top of the new code, I have yet to see it
(meaning you didn't include it in the patch series.)

To get me to take these changes, you have to show a real need and user
of the code.  Without that it just strongly looks like you are having
fun making a more complex api for no reason that we are then going to be
stuck with maintaining.

So clean away, and fix up, but remember, you have to be able to justify
each change as being needed.  And so far, I'm not sold on this at all,
sorry.

thanks,

greg k-h

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

#1668866

On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote:

> But we don't accept kernel patches for some mythical future option
> that might be happening some time in the future.  Heck, I'm still not
> convinced that firmware signing isn't anything more than just some
> snakeoil in the first place!

I for one really want the "firmware" signing, because I want to load
the regulatory database through this API, and 

But honestly, I've been waiting for years for that now and started
looking at what it would take to hand-implement that on top of the
existing firmware API. Probably not all that much.

johannes

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

#1669880

On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote:
> On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote:
> 
> > But we don't accept kernel patches for some mythical future option
> > that might be happening some time in the future.  Heck, I'm still not
> > convinced that firmware signing isn't anything more than just some
> > snakeoil in the first place!
> 
> I for one really want the "firmware" signing, because I want to load
> the regulatory database through this API, and 

This was my original goal as well... and it was also one of the reasons why
the API name change would be much better reflective of future possible uses.

> But honestly, I've been waiting for years for that now and started
> looking at what it would take to hand-implement that on top of the
> existing firmware API. Probably not all that much.

I had proposed changes to do just this long ago, without any new *API*, so we'd
support firmware signing just as we do with module signing. Simple!

It was during these discussions that we realized we actually *wanted* to have
the option to always specify requests with specific signing requirements from
the start, as such a flexible API became a prerequisite and so I prioritized
that work first.

Lets not ignore previous work and prior discussions then, the last effort on this
front was by AKASHI, and it'd be greatly appreciated if the topic of firmware
signing was specifically addressed on that thread there [0].

[0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org

 Luis

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

#1670147

On Mon, Jun 19, 2017 at 09:41:07PM +0200, Luis R. Rodriguez wrote:
> On Mon, Jun 19, 2017 at 09:33:16AM +0200, Johannes Berg wrote:
> > On Sat, 2017-06-17 at 21:38 +0200, Greg KH wrote:
> > 
> > > But we don't accept kernel patches for some mythical future option
> > > that might be happening some time in the future.  Heck, I'm still not
> > > convinced that firmware signing isn't anything more than just some
> > > snakeoil in the first place!
> > 
> > I for one really want the "firmware" signing, because I want to load
> > the regulatory database through this API, and 
> 
> This was my original goal as well... and it was also one of the reasons why
> the API name change would be much better reflective of future possible uses.
> 
> > But honestly, I've been waiting for years for that now and started
> > looking at what it would take to hand-implement that on top of the
> > existing firmware API. Probably not all that much.
> 
> I had proposed changes to do just this long ago, without any new *API*, so we'd
> support firmware signing just as we do with module signing. Simple!
> 
> It was during these discussions that we realized we actually *wanted* to have
> the option to always specify requests with specific signing requirements from
> the start, as such a flexible API became a prerequisite and so I prioritized
> that work first.
> 
> Lets not ignore previous work and prior discussions then, the last effort on this
> front was by AKASHI, and it'd be greatly appreciated if the topic of firmware
> signing was specifically addressed on that thread there [0].

+1
I always appreciate any comments from those who are for and against
my patch (or firmware signing in general) as well.

-Takahiro AKASHI


> [0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org
> 
>  Luis

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

#1669838

On Sat, Jun 17, 2017 at 09:38:15PM +0200, 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 :)

Great!

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

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.

> Again, the code is now bigger, does more, with not even any real benefit
> for existing users.

Obviously I disagree strongly, in light of the history of the code. Not only
should the existing code be compared but also how it has evolved and we should
evaluate whether or not its evolution can be dealt with more appropriately.

> > If not, what concrete alternatives do you suggest?
> 
> It's working, so leave it alone?  :)

As the maintainer of the firmware API I **totally** disagree!!

First the firmware API has been evolving as pieces of gum thrown at it. A
proper design of the API in consideration of extensions is in order.  Likewise
goes for testing and documentation. Second, it cannot be said with any serious
tone that the current design has worked well.  It is simply not an accurate
reflection of the codebase. The design of the fallback mechanism and the amount
of issues that have gone through it is a great example.

The issues I have fixed along the way, and the chaotic way in which the API
has evolved have put me to reflect well on a proper design of the firmware API.

> > > :(
> > > 
> > > Seriously, why?  Why are we extending any of this at all? 
> > 
> > Addressing easy extensibility was a prerequisite of considering firmware signing
> > support. Its really the *only* reason I started reviewing the firmware API, to the
> > point I started helping fix quite a bit of bugs and now just became the maintainer.
> > 
> > So my original firmware signing effort, now driven by AKASHI Takahiro, was one
> > of the original motivations here!
> 
> But we don't accept kernel patches for some mythical future option that
> might be happening some time in the future.

Greg, these are non mythical "future options" -- they are real feature requests
and they have all valid reasons and discussions ongoing on the mailing list. The
streaming FPGA support is a concrete valid use case we need to extend *now* and
your reply brilliantly seems to have completely ignored it.

I do agree that firmware signing itself was a largely debatable topic, there
were *really-really* long threads on the topic. However we had a hallway track
at Santa Fe Plumbers where it seemed we reached consensus on a path forward!
The firmware signing topic should be addressed on TAKASHI's patch set [0]. Please
address your NACK and reasons there. Even though it is based on the driver
data API, the main topic points of *logic* for it can *also* be discussed
there.

BUT NOTE:

Before we could move forward with firmware signing we need also a *sane* way to
extend the API to address the needs of firmware signing. So a proper sane API
for future extensions is in order as a prerequisite. This goes along with
testing and documentation.

This goes for *any* new firmware API feature!!!

Firmware signing was just one example feature!!!

Also -- even if they were "mythical" the *point* of this series is to address
extensibility of the API, and so one of the ways to measure the gains of the
design of a new API is to look at what the code would look like when new
features are added. This is why I pointed out to review these proposed changes.

Even more so given I am noting that these are non-mythical features!!

[0] https://lkml.kernel.org/r/20170526030609.1414-1-takahiro.akashi@linaro.org

> Heck, I'm still not convinced that firmware signing isn't anything more than
> just some snakeoil in the first place!

Sure, some may have similar sentiments, this topic should be addressed
separately on TAKASHI's latest patches [0]. Please address this on the patches
posted, Otherwise I really am afraid we would conflating discussions and
confusing the core issue on this thread:

   The future direction of extensibility of the firmware API!

> So while you mention lots of times that all sorts of wonderful things
> can now possibly be built on top of the new code, I have yet to see it
> (meaning you didn't include it in the patch series.)

Clearly I disagree, but perhaps the issue here may be conflating "addressing
extensibility of the firmware API" with actual features.

> To get me to take these changes, you have to show a real need and user
> of the code.

The *core issue* here seems to be that the features for which we have had to
consider new extensions of the firmware API have been debatable, so my arguments
for proper design are being conflated with the features introduced at the time
the redesign is introduced.

The driver data API was originally introduced with firmware signing, and the
topic of firmware signing alone was a hugely debatable topic. Even though I
note those debatable issues were resolved its fair to still question them, but
those must be addressed separately.

For this reason I looked at the next feature on my radar to consider which we
needed which was non-debatable. The next closest feature was that of making
async firmware requests optional, just as request_firmware_direct() exist for
sync requests.  Unfortunately the *need* for this *just disappeared* given that
it was clarified by Hans de Geode that the driver that *needed* this, brcmfmac,
no longer optionally needs the firmware [1]. This is precisely why I dropped
the respective brcmfmac changes in my last iteration of the driver data API.
Even though I dropped the actual driver use, I kept the feature as its
obviously sensible!

But anyway, my point is that this *reason* vanished from upstream all of a
sudden...

Next, as the firmware maintainer, I knew addressing daisy chained requests for
a series of firmware revisions was another *good idea* to address upstream
properly, given this can be complex and the amount of issues that could lurk
there.  You have foo driver which supports a series of ranges of API firmware
X-Y, we should allow for an API which enables this in a easy way.  A good
example complex case was in the iwlwifi driver, it actually uses recursion to
accomplish the above objectives. Despite the recursion design, their layout for
expectations for the file names for a series of API revisions seems very
sensible to me. Its a flexible layout I think we can stick to and support
generically. As such I implemented this on the firmware API and made the
implementation deterministic, avoiding recursion.

This feature alone should be weighed on its own. While I do agree it would seem
to appear we only have *one upstream* user right now, I cannot imagine that
this is generally true, daisy chaining requests on a series of revisions seems
to me a logical approach and I expect much more users may exist, its just a
matter hunting and conversion.

You may argue that *one* upstream users is not sufficient to introduce a new
feature for, but I disagree given we have had new full *API* added for a new
feature on the firmware API even for drivers THAT ARE NOT UPSTREAM! For
instance request_firmware_into_buf() has no upstream users!!!

Now, you might say that even though this is true that there many users of
out-of-tree drivers that need this. While true, if this is the bar we'd go
with, we can't then ignore the iwlwifi userbase, and the possible gains of
having a proper non-recursive use of the daisy chained requests.

Also, its *precisely* this loose API garbage such as what went in with
request_firmware_into_buf() which I'm trying to avoid. Loosely adding API with
hidden features is *not a good idea*, can lead to further misprogramming or odd
bugs. Worst it also paves the way to further sloppy extensions. For instance
the request_firmware_into_buf() API makes use of a no-cache feature which
we *also* can have a use for on *existing* drivers upstream such as iwlwifi
which deal with *caching on its own* ! Its also a requirement for streaming
API for FPGAs.

Also it should be considered that other than addressing the extensibility of
the firmware API, the driver data API intentionally strives to bundle specific
unit tests for *each* feature being introduced. This also goes with a philosophy
re-design of the test driver: instead of making *one* knob per test case in the
C test driver, I am designing the test case configuration completely in
userspace, striving to allow us to build *any* possible test case form
userspace.

[1] https://lkml.kernel.org/r/09063fc2-af77-ced6-ed90-ab20e2884969@redhat.com

> Without that it just strongly looks like you are having
> fun making a more complex api for no reason that we are then going to be
> stuck with maintaining.

Greg, this is rather insulting considering the amount of work I have put
into thought about proper design of the firmware API in consideration of
past issues and its rather loose grotesque evolution which I have been
alerting to and pointing out.

> So clean away, and fix up, but remember, you have to be able to justify
> each change as being needed.  And so far, I'm not sold on this at all,
> sorry.

I have put a lot of work into a proper design here in consideration of the
entire history of the firmware API. We *need* a proper architectural design
for the firmware API to evolve it in ways that does not allow folks to just
throw gum at it. We also want to build test units for each new feature and
ensure *each new feature* is properly documented. That is what I have provided
in this patch series.

  Luis

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

#1673827

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] | [prev] | [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]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web