On Wed, Aug 03, 2016 at 05:55:40PM +0200, Luis R. Rodriguez wrote: > 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. A file sentinel would implicate a file namespace thing being used on the filesystem -- to me this just means the Linux distribution / system integrator would add this per filesystem, but agree this is pretty hacky. Furthermore we'd wait forever if the Linux distribution / system integrator forgot to set the sentinel file. That's not good. To avoid that a generic "root fs ready" event could be sent from userspace to know when to clear stale reads... but if that's going to be done best just replace all sentintels with a simple "root fs ready" which would mean all reads from the kernel are ready. If we wanted further granularity I suppose we could further have one event per enum kernel_read_file_id, and a generic all-is-ready one. To start off with then a simple event from userspace should suffice. But do keep in mind that granularity might help given that a big iron system might have some large array of disks to mount during bootup and that may take a long while, and you likely want to read /lib/firmware way before that filesystem is ready. Not sure if granularity fixated by enum kernel_read_file_id should suffice, perhaps given its also enough for LSMs... This indeed would mean a kernelspace and userspace change, but it would mean not having to deal with the usermode helper crap anymore. Anyway -- these are just ideas, patches welcomed ! 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
