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 <mcg...@kernel.org> >>> 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. 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. Does the "rootfs ready" event cover suspend/resume? > 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? >>> 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. >>> 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-mcg...@kernel.org > > 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 Regards, Arend
