On Dinsdag 08 März 2005 23:37, Michal Januszewski wrote:

On Tue, Mar 08, 2005 at 04:18:07AM +0100, Arnd Bergmann wrote:

quoted

It should probably just use its own hotplug agent instead of calling
the helper directly.

I've just had a look at it, and it seems possible. From what I have seen
in the firmware_class.c code, it would require:
 - registering a class somewhere in the initializaton code
 - every time a request from fbcon is generated:
   - register the class device
   - create a timer
   - call kobject_hotplug() to send the event to userspace
   - unregister the device

This requests sent to userspace are generated while switching consoles
and resetting the graphics mode. This whole create device - send the
event - remove device thing seems a little long to me. Would it be
acceptable?

Sorry, I had forgotten about the recent changes to the kernel side of the
hotplug interface, i.e. that it now needs a kobject to work.
It used to be possible to just call_usermodehelper() with hotplug_path
as its argv[0], but that is currently being phased out.

You also shouldn't create a class device every time you want to call
kobject_hotplug. Note that every character device already has a class
device associated with it, so you might be able to just use that to call 
kobject_hotplug on it.

If there is no obvious way to do that, just leave the code as it is
for calling the helper.

quoted

quoted

+ When the userspace helper is called in an early phase of the boot process
+ (right after the initialization of fbcon), no filesystems will be mounted.
+ The helper program should mount sysfs and then create the appropriate 
+ framebuffer, fbsplash and tty0 devices (if they don't already exist) to get 
+ current display settings and to be able to communicate with the kernel side.
+ It should probably also mount the procfs to be able to parse the kernel 
+ command line parameters.

Nothing about the init command seems really necessary. Why not just do 
that stuff from an /sbin/init script?

The reason for calling init in that place is the ability to use the
userspace helper to display a nice picture while the kernel is still
loading. Sure, you can just drop it and use the 'quiet' command line
option, but that will give you a black screen for a good few seconds.
And people who want eye-candy like fbsplash generally don't like that. 

Ok, understood. I think you should make that an extra patch and completely
remove that feature from the base patchset in order to make it less
controversial.

quoted

quoted

+FB_SPLASH_IO_ORIG_KERNEL instructs fbsplash not to try to acquire the console
+semaphore. Not surprisingly, FB_SPLASH_IO_ORIG_USER instructs it to acquire
+the console sem.

That sounds really dangerous. Can you guarantee that nothing unexpected happens
when a malicious user calls the ioctls with FB_SPLASH_IO_ORIG_KERNEL from a
regular user process?

This is pretty nasty, right, but I just can't see a way around it. The
thing is, when the splash helper is called from the kernel, the console
semaphore is already held so we have to avoid trying to acquire it
again. And we can't just release it prior to calling the helper, as it
would break things badly.

I don't understand. Does the kernel code need to wait for the helper
to finish while holding the semaphore? AFAICS, the helper is completely
asynchronous, so it can simply do its job when the kernel has given
up the semaphore.

quoted

quoted

+Info on used structures:
+
+Definition of struct vc_splash can be found in linux/console_splash.h. It's
+heavily commented. Note that the 'theme' field should point to a string

Not that heavily documented, actually ;-). 

Anything that needs some clearing-up?

No, probably not. Maybe just remove that sentence from the text.

quoted

quoted

+no longer than FB_SPLASH_THEME_LEN. When FBIOSPLASH_GETCFG call is 
+performed, the theme field should point to a char buffer of length
+FB_SPLASH_THEME_LEN.

Then don't make it pointer but instead a field of that length. Pointers
in ioctl arguments only cause trouble in mixed 32/64 bit environments.

That could be arranged, but this would require a separate structure for
communicating with userspace and with in-kernel data storage (currently
both these tasks are handled with struct vc_splash), or changing
vc_splash in vc_data to a pointer to a structure. The latter option
would be better I think.

Yes, that makes sense.

- fbsplash calls aren't really related to a particular framebuffer
  device, they are related to a tty. 

ok.

                                      And we can't do ioctls on ttys when 
  answering a kernel request because to the console sem problems
  (opening a tty = a call to acquire_console_sem(), which we need to
  avoid).

Hmm. One of us has misunderstood the interaction between 
call_usermodehelper() and acquire_console_sem(). If I'm the one
who's wrong, please tell me where that semaphore is held.

The original idea behind this was to group the fields that are common to
all fbsplash ioctl calls (ie. vc and origin) in one place. Of course,
it can be changed to three differents structs, each containing the vc
and origin fields and an int/struct vc_splash/struct fb_image, if that
is the preferred way of doing things.

It definitely is. Actually, it would be preferred to have only a single
value as ioctl argument (or even better, no ioctl at all), but if you
need to pass an aggregate data type, it should at least have an identical
layout for all architectures. That means every member should be naturally
aligned and you can't use pointers or other types that have sizeof(x)
== sizeof(long).

 Arnd <><