Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Wed, Aug 10, 2016 at 09:04:38PM +0200, Arend Van Spriel wrote:
> On 10-8-2016 20:32, Luis R. Rodriguez wrote:
> > On Fri, Jun 17, 2016 at 08:35:03PM +0200, Luis R. Rodriguez wrote:
> >> On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
> >>> On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> >>>>
> >>>> Reason this could not wait is folks seem to want to keep extending the API,
> >>>> which is another reason for this, do we want to put an end to an unflexible
> >>>> API now or should we wait ?
> 
> [big snip]
> 
> > 
> > Regarding this -- Dmitry recenlty noted devm only works if used on the probe
> > path, and as we now determined, we don't want firmware loading on probe [3], unless
> > async probe is used, so this would make a devm solution here not ideal for
> > freeing the firmware. Even if you use async probe -- that seems very special
> > use case and adding devm support for the firmware API just for that seems silly.
> 
> So why would drivers want the devm solution anyway.

It depends on the use case, some may want to keep the firmware around,
some may not, but by default the new API ssumes you will not (keep = false)
and we free it. My point above was that using devm will not typically
be the most fruitful solution to free the firmware givne that there are only
a few drivers that should need it upon probe, and drivers using firmware
APIs on probe shoudld be using async probe anyway to delay avoid delaying
boot as processing the firmware can take time.

> Once firmware is
> loaded in the device it can be freed or do you expect device drivers to
> keep the firmware/sysdata for suspend/resume scenario as some do because
> of firmware cache behaviour.

You would think! Upon careful inspection there are tons of odd things drivers
do, some modify the firmware and therefore have their own reasons to keep
it. Whatever the reasons are -- I recall seeing a few well justified uses.

> Does the "rootfs ready" event cover
> suspend/resume?

The "rootfs ready" is orthogonal to suspend/resume case given the firmware_class
cache firmware feature.

The firmware API as-is upstream already has a cache firmware solution added
a long time ago, as reflected by a resent patch set (before this one) I
updated documentation for this given its not clearly well known and people
keep adding their own caching solutions, the firmware API requests firmware
prior to suspend so that upon resume the firmware is present, precisely to
avoid race issues. I will note that this feature is only for non-usermode helper
firmware API, upon suspend we *kill* any pending user mode helper requests
given that this can delay suspend, as such drivers using or relying (only
2) on the user mode helper for firmware have no solution built in for
the cache stuff and I can't say I care given there seems to be no valid
modern use case given as a requirement for it yet. In fact there was a bug
in the firmware_class code that *allows* the cache call to request the
usermode helper, obviously that's buggy if we are trying to kill the
usermode helper upon suspend...  so a pending patch fixes that. That's been
a long standing bug and surprised no one ever picked up on it.

> > As such the current devised solution in the sysdata API is called for, given
> > if you indicated that keep = false, you are explicitly telling the firmware
> > API that your firmware will certainly not be needed after the callback is
> > called.
> > 
> > So for the sync case, a new callback is needed, and that explains the extra
> > bit of code if someone conerts from the old API to the new one.
> > 
> > [3] https://lkml.kernel.org/r/20160803161821.GB32965@dtor-ws
> > 
> >>> or a magical "sysdata_desc" descriptor,
> >>
> >> This is one way to make a flexible and extensible API without affecting drivers
> >> with further collateral evolutions as it gets extended. Its no different than
> >> the "flags" lesson learned from writing system calls, for instance.
> >>
> >> Descriptor seemed, fitting, let me know if you have any other preference.
> > 
> > I haven't heard otherwise so will be sticking to that.
> 
> How about sysdata_req{,uest}_params?

Thanks will go with that.

> >>> and having a new name ("sysdata") that is less descriptive than the old one
> >>> ("firmware")
> >>
> >> Well no, the APIs are used in *a lot* of cases for things that are not firmware
> >> already, and let's recall I originally started working on this to replace CRDA
> >> from userspace to be able to just fetch the signed regulatory database file
> >> from the kernel. Calling it firmware simply makes no sense anymore.
> > 
> > So help me bike shed. This seems to be sticking point and I frankly don't
> > care what we call it. I've asked others for name suggestions and here are
> > a few suggestions:
> > 
> >  o driver_data
> >  o dsd: device specific data
> >  o xfw - eXtensible firmware API
> >  o bbl - binary blob loader
> > 
> > Can someone just pick something? I really, really do not care.
> > 
> > If I don't hear anything concrete I will go with driver_data.
> 
> bit of skin crawling here, but not enough to care.

Bike shedding is so much fun. Not.

> >>> are all in my opinion making the example patch be a
> >>> step _backwards_ rather than an improvement. It does not look like a
> >>> simpler or more natural interface for a driver.
> >>
> >> Hope the above explains the current state. Feedback on desirable changes
> >> welcomed.
> >>
> >> [0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@xxxxxxxxxx
> > 
> > All this said, this series is still justified, the extra code only comes in
> > place when porting the sync requests due to the callback stuff described above
> > and the inability to use devm there. As far as I can tell, just the bike
> > shedding is left.
> > 
> > As it stands then, unless I hear back, I'll roll Daniel Wagner's changes into
> > my series to be applied first, then rename sysdata driver_data, rebase all this
> > and shoot it out again.
> > 
> > Only a few drivers will be converted over as demos. The SmPL grammar can be used
> > by those who do want a change due to the few features added. Long term we'll
> > add more features to the new API:
> > 
> >  o the whole ihex conversion is crap, we should do this within the API and
> >    this can just be specified as a descriptor preference, then drivers
> >    don't have to deal with the ihex crap themselves.
> > 
> >  o firmware singing - this lets us kill CRDA as a requirement
> 
> Strongly suspect a typo here :-p

Indeed, thanks :)

  Luis