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

[Vikram Mulukutla]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx
>
> 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