Messages in this thread |  | | Date | Sat, 21 Apr 2018 19:36:50 +0200 | From | "Luis R. Rodriguez" <> | Subject | Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4 |
| |
On Sat, Apr 21, 2018 at 08:32:00AM -0700, Linus Torvalds wrote: > On Sat, Apr 21, 2018 at 8:11 AM, Kees Cook <keescook@chromium.org> wrote: > > > > What was the objection to using parameters for this? i.e. something > > like the gfp flags, but have a behavior flag FW_RQ_NOWAIT, > > FW_RQ_NOWARN, etc? > > The objection was that the patches that I think Luis refers to
There were different topics of discussion, people can conflate the topics discussed easily as all these topics were discussed around the same time so best to recap and split them up:
a) bikeshedding on the name: I suggested long ago we've already passed usage of the fw API to things which are *not firmware*, so suggested we use a new naming scheme, the last one I recommended was a driver_data_*() prefix. Greg last suggested a firmware_*() prefix -- and to rename *all* existing callers with this new prefix as well long term... I am done with bikeshedding and frankly don't care anymore. Whatever you folks decide, I just want to move on with life, so I am moving along with the firmware_*() prefix.
b) evolving the API: data driven Vs functional -- where to draw the fine line -- this is the topic we are discussing now again --
c) firmware signing: we've decided against it for now and folks who need it can open code it, mainly due to most hw supporting FW signing already
> (a) passed in a union of random arguments
It split the API into two main routine calls, a sync and an async call, and also accepted a *structure of parameters* which describe the request requirements [0].
It turned out this approach was *too data driven*, and Greg advised against it, asking for a new call *per new functionality*, as is typically done...
[0] https://lkml.kernel.org/r/20170605213937.26215-2-mcgrof@kernel.org
> (b) changed all the users, even the ones that didn't want to be changed.
That *never happened* actually. The SmPL grammar I provided long ago was for those who *did* want to change the new API if they had a new *feature* they wanted to take advantage of. The patches you may have seen were *examples* of *two drivers* which were converted over just an example with the grammar.
On the last iteration of my series I skipped adding the SmPL helpers, and only converted *one* driver for *new functionality*.
In the last driver data patch series I only converted iwlwifi as I was changing it to use the new functionality to replace its own internal recursion set of calls with an internal deterministic solution which lets drivers call for a range of API files and lets it use the first one in a range it finds [1], with this diff stat:
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 91 ++++++++++------------------ 1 file changed, 31 insertions(+), 60 deletions(-)
[1] https://lkml.kernel.org/r/20170605213937.26215-6-mcgrof@kernel.org
> they were nasty and illegible and pointless.
Clearly request_firmware_nowait2() is *not* much better... right? So it illustrates the problem I was hinting which we'd eventually cross...
> Using some single flag field for an extended function, and leaving the > existing functions alone so that you don't have to convert existing > users - that would have been fine. That's not what was tried and > rejected.
Actually it was tried, however the divide was perhaps *too broad* and split all possible *new* functionality into two calls, a sync and a async call.
A flag based mechanism *is* reasonable to me given I have been an advocate of such type of mechanism for a long while. It however is against what Greg requested -- to have a new call *per functionality*.
So feel free to advise... I really just want us to move on.
Luis
|  |