Re: [announce 7/7] fbsplash - documentation

On Wed, Mar 09, 2005 at 01:17:11AM +0100, Arnd Bergmann wrote:

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.

OK, thanks forthe explanation and advice. I'll look into it.

quoted

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.

That can be done. The funny thing is, a week ago or so, it was proposed
in a thread about bootsplash that the code for the initial call could be
merged first and serve as a base for merging fbsplash :)

quoted

quoted

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

                                    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.

It think this will be best explained with an example. Consider the
following situation - we have two ttys - tty1 and tty2. tty1 is using
theme 'foo' an it's the foreground console. tty2 is using theme 'bar'.
Fbsplash is enabled on both these ttys.

Now the user decides to switch to tty2. He/she presses Alt+F2. The
keypress is handled somewhere and an item is added to the console_work
workqueue. console_callback() gets called asynchronously.
acquire_console_sem() is the first thing done in that function.

Now we have the following call stack (all done with the console sem
held):

change_console()
complete_change_console()
switch_screen() == redraw_screen()
con_switch() == fbcon_switch()

From inside fbcon_switch(), we need to call the splash helper to get a
new picture for the theme 'bar' which is used on tty2. The splash helper
does an ioctl and we're back in the kernel.. with the console semaphore
already held.

quoted

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).

What are the alternatives to the ioctl? A relatively clean way of
feeding the data back to the kernel would be using sysfs. However, to
make it sane we would have to export the various components of struct
vc_splash in /sys/class/tty/tty<x> (otherwise, we would probabky have
to pass aggreaget data types through a sysfs entry -- something that is
IMO much worse than an ioctl). That however could be a little
problematic -- tty_class is currently defined as a class_simple.
To add entries other than the standard 'dev' we would have to define a
completely new class, right? That sounds like a rather intrusive
change..

Live long and prosper.
-- 
Michal 'Spock' Januszewski                        Gentoo Linux Developer
cell: +48504917690                         http://dev.gentoo.org/~spock/
JID: spock@im.gentoo.org               freenode: #gentoo-dev, #gentoo-pl