Messages in this thread | | | Date | Mon, 26 Jun 2017 20:19:07 +0200 | From | Rafał Miłecki <> | Subject | Re: [PATCH v9 1/5] firmware: add extensible driver data params |
| |
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).
| |