On Wed, Aug 03, 2016 at 08:57:09AM +0200, Daniel Wagner wrote: > On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote: > >On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote: > >>On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote: > >>>On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote: > >>So you argue for the remoteproc use case with 100+ MB firmware that > >>if there is a way to load after pivot_root() (or other additional > >>firmware partition shows up) then there is no need at all for > >>usermode helper? > > > >No, I'm saying I'd like to hear valid uses cases for the usermode helper and so > >far I have only found using coccinelle grammar 2 explicit users, that's it. My > >patch series (not yet merge) then annotates these as valid as I've verified > >through their documentation they have some quirky requirement. > > I got that question wrong. It should read something like 'for the > remoteproc 100+MB there is no need for the user help?'. That's not a question for me but for those who say that the usermode helper is needed for remoteproc, so far from what folks are saying it seems the only reason for the usermodehelper was to try to avoid the deterministic issue, but I suggested a way to resolve that without the usermode helper now so would be curious to hear if there are any more reasons for it. > I've gone > through your patches and they make perfectly sense too. Maybe I can > convince you to take a better version of my patch 3 into your queue. > And I help you converting the exiting drivers. Obviously if you like > my help at all. I accept all help and would be glad to make enhancements instead of the old API through new API. The biggest thing here first I think is adding devm support, that I think should address what seemed to be the need to add more code for a transformation into the API. I'd personally only want to add that and be done with an introduction of the sysdata API. Further changes IMHO are best done atomically after that on top of it, but I'm happy to queue in the changes. > >Other than these two drivers I'd like hear to valid requirements for it. > > > >The existential issue is a real issue but it does not look impossible to > >resolve. It may be a solution to bloat up the kernel with 100+ MB size just to > >stuff built-in firmware to avoid this issue, but it does not mean a solution > >is not possible. > > > >Remind me -- why can remoteproc not stuff the firmware in initramfs ? > > I don't know. I was just bringing it up with the hope that Bjorn > will defend it. It seems my tactics didn't work out :) OK. > >Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor > >support per enum kernel_read_file_id. For instance we'd have one for > >READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this > >would in turn be used as the system configurable deterministic file for > >which to wait for to be present before enabling each enum kernel_read_file_id > >type read. > > > >Thoughts ? > > Not sure if I get you here correctly. Is the 'system configurable > deterministic file' is a knob which controlled by user space? Or it > this something you define at compile time? I meant at compile time on the kernel. So CONFIG_READ_READY_SENTINEL or something like this, and it be a string, which if set then when the kernel read APIs are used, then a new API could be introduced that would *only* enable reading through once that sentinel has been detected by the kernel to allowed through reads. Doing this per mount / target filesystem is rather cumbersome given possible overlaps in mounts and also pivot_root() being possible, so instead targeting simply the fs/exec.c enum kernel_read_file_id would seem more efficient and clean but we would need a decided upon set of paths per enum kernel_read_file_id as base (or just one path per enum kernel_read_file_id). For number of paths I mean the number of target directories to look for the sentinel per enum kernel_read_file_id, so for instance for READING_FIRMWARE perhaps just deciding on /lib/firmware/ would suffice, but if this supported multiple paths another option may be for the sentinel to also be looked for in /lib/firmware/updates/, /lib/firmware/" UTS_RELEASE -- etc. It would *stop* after finding one sentinel on any of these paths. If a system has has CONFIG_READ_READY_SENTINEL it would mean an agreed upon system configuration has been decided so that at any point in time reads against READING_FIRMWARE using a new kernel_read_file_from_path_sentinel() (or something like it) would only allow the read to go through once the sentinel has been found for READING_FIRMWARE on the agreed upon paths. The benefit of the sentintel approach is it avoids complexities with pivot_root(), and makes the deterministic aspect of the target left only to a system-configuration enabled target path / file. This is just an idea. I'd like some FS folks to review. > Hmm, so it would allow to decided to ask a userspace helper or load > the firmware directly (to be more precised the kernel_read_file_id > type). If yes, than it is what currently already have just > integrated nicely into the new sysdata API. Sorry, no, the above description is better of what I meant. This actually would not need to go into the sysdata API, unless of course we wanted it just as a new "feature" of it, but I don't think that's needed unless it has some implications behind the scenes. Given that firmware_class now uses a common core kernel API for reading files kernel_read_file_from_path() we could for instance add kernel_read_file_from_sentintel() and only if CONFIG_READ_READY_SENTINEL() would it block and wait until the sentinel clears. This should mean being able to make the change for both the old API and the new proposed sysdata API. Likewise for other kernel_read_file*() users -- they'd benefit from it as well. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
