Signed-off-by: Michael Januszewski <[email protected]>
---
diff -Nru a/Documentation/fb/00-INDEX b/Documentation/fb/00-INDEX
--- a/Documentation/fb/00-INDEX 2005-03-07 16:50:34 +01:00
+++ b/Documentation/fb/00-INDEX 2005-03-07 16:50:34 +01:00
@@ -19,6 +19,8 @@
- info on the Matrox frame buffer driver
pvr2fb.txt
- info on the PowerVR 2 frame buffer driver
+splash.txt
+ - info on the Framebuffer Splash
tgafb.txt
- info on the TGA (DECChip 21030) frame buffer driver
vesafb.txt
diff -Nru a/Documentation/fb/splash.txt b/Documentation/fb/splash.txt
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/Documentation/fb/splash.txt 2005-03-07 16:50:34 +01:00
@@ -0,0 +1,207 @@
+What is it?
+-----------
+
+The framebuffer splash is a kernel feature that allows displaying a background
+picture on selected consoles.
+
+What do I need to get it to work?
+---------------------------------
+
+To get fb splash up-and-running you will have to:
+ 1) get a copy of splashutils [1] or a similar program
+ 2) get some splash themes
+ 3) build the kernel helper program
+ 4) build your kernel with the FB_SPLASH option enabled.
+
+To get fb splash operational right after fbcon initialization is finished, you
+will have to include a theme and the kernel helper into your initramfs image.
+Please refer to splashutils documentation for instructions on how to do that.
+
+[1] The splashutils package can be downloaded from:
+ http://dev.gentoo.org/~spock/projects/splashutils/
+
+The userspace helper
+--------------------
+
+The userspace splash helper (by default: /sbin/splash_helper) is called by the
+kernel whenever an important event occurs and the kernel needs some kind of
+job to be carried out. Important events include console switches and graphic
+mode switches (the kernel requests background images and configuration
+parameters for the current console). The splash helper must be accessible at
+all times. If it's not, fbsplash will be switched off automatically.
+
+It's possible to set path to the splash helper by writing it to
+/proc/sys/kernel/fbsplash.
+
+*****************************************************************************
+
+The information below is mostly technical stuff. There's probably no need to
+read it unless you plan to develop a userspace helper.
+
+The splash protocol
+-------------------
+
+The splash protocol defines a communication interface between the kernel and
+the userspace splash helper.
+
+The kernel side is responsible for:
+
+ o rendering console text, using an image as a background (instead of a
+ standard solid color fbcon uses),
+ o accepting commands from the user via ioctls on the fbsplash device,
+ o calling the userspace helper to set things up as soon as the fb subsystem
+ is initialized.
+
+The userspace helper is responsible for everything else, including parsing
+configuration files, decompressing the image files whenever the kernel needs
+it, and communicating with the kernel if necessary.
+
+The splash protocol specifies how communication is done in both ways:
+kernel->userspace and userspace->helper.
+
+Kernel -> Userspace
+-------------------
+
+The kernel communicates with the userspace helper by calling it and specifying
+the task to be done in a series of arguments.
+
+The arguments follow the pattern:
+<splash protocol version> <command> <parameters>
+
+All commands defined in splash protocol v2 have the following parameters:
+ virtual console
+ framebuffer number
+ theme
+
+Splash protocol v1 specified an additional 'fbsplash mode' after the
+framebuffer number. Splash protocol v1 is deprecated and should not be used.
+
+Splash protocol v2 specifies the following commands:
+
+getpic
+------
+ The kernel issues this command to request image data. It's up to the userspace
+ helper to find a background image appropriate for the specified theme and the
+ current resolution. The userspace helper should respond by issuing the
+ FBIOSPLASH_SETPIC ioctl.
+
+init
+----
+ The kernel issues this command after the fbsplash device is created and
+ the fbsplash interface is initialized. Upon receiving 'init', the userspace
+ helper should parse the kernel command line (/proc/cmdline) or otherwise
+ decide whether fbsplash is to be activated.
+
+ To activate fbsplash on the first console the helper should issue the
+ FBIOSPLASH_SETCFG, FBIOSPLASH_SETPIC and FBIOSPLASH_SETSTATE commands,
+ in the above-mentioned order.
+
+ 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.
+
+ Note that the console sem is not held when the kernel calls splash_helper
+ with the 'init' command. The splash helper should perform all ioctls with
+ origin set to FB_SPLASH_IO_ORIG_USER.
+
+modechange
+----------
+ The kernel issues this command on a mode change. The helper's response should
+ be similar to the response to the 'init' command. Note that this time the
+ console sem is held and all ioctls must be performed with origin set to
+ FB_SPLASH_IO_ORIG_KERNEL.
+
+
+Userspace -> Kernel
+-------------------
+
+Userspace programs can communicate with fbsplash via ioctls on the fbsplash
+device. These ioctls are to be used by both the userspace helper (called
+only by the kernel) and userspace configuration tools (run by the users).
+
+The splash helper should set the origin field to FB_SPLASH_IO_ORIG_KERNEL
+when doing the appropriate ioctls. All userspace configuration tools should
+use FB_SPLASH_IO_ORIG_USER. Failure to set the appropriate value in the origin
+field when performing ioctls from the kernel helper will most likely result
+in a console deadlock.
+
+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.
+
+The framebuffer splash provides the following ioctls (all defined in
+linux/fb.h):
+
+FBIOSPLASH_SETPIC
+description: loads a background picture for a virtual console
+argument: struct fb_splash_iowrapper*; data: struct fb_image*
+notes:
+If called for consoles other than the current foreground one, the picture data
+will be ignored.
+
+If the current virtual console is running in a 8-bpp mode, the cmap substruct
+of fb_image has to be filled appropriately: start should be set to 16 (first
+16 colors are reserved for fbcon), len to a value <= 240 and red, green and
+blue should point to valid cmap data. The transp field is ingored. The fields
+dx, dy, bg_color, fg_color in fb_image are ignored as well.
+
+FBIOSPLASH_SETCFG
+description: sets the fbsplash config for a virtual console
+argument: struct fb_splash_iowrapper*; data: struct vc_splash*
+notes: The structure has to be filled with valid data.
+
+FBIOSPLASH_GETCFG
+description: gets the fbsplash config for a virtual console
+argument: struct fb_splash_iowrapper*; data: struct vc_splash*
+
+FBIOSPLASH_SETSTATE
+description: sets the fbsplash state for a virtual console
+argument: struct fb_splash_iowrapper*; data: unsigned int*
+ values: 0 = disabled, 1 = enabled.
+
+FBIOSPLASH_GETSTATE
+description: gets the fbsplash state for a virtual console
+argument: struct fb_splash_iowrapper*; data: unsigned int*
+ values: as in FBIOSPLASH_SETSTATE
+
+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
+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.
+
+Definition of struct fb_splash_iowrapper can be found in linux/fb.h.
+The fields in this struct have the following meaning:
+
+vc:
+Virtual console number.
+
+origin:
+Specifies if the ioctl is performed as a response to a kernel request. The
+splash helper should set this field to FB_SPLASH_IO_ORIG_KERNEL, userspace
+programs should set it to FB_SPLASH_IO_ORIG_USER. This field is necessary to
+avoid console semaphore deadlocks.
+
+data:
+Pointer to a data structure appropriate for the performed ioctl. Type of
+the data struct is specified in the ioctls description.
+
+*****************************************************************************
+
+Credit
+------
+
+Original 'bootsplash' project & implementation by:
+ Volker Poplawski <[email protected]>, Stefan Reinauer <[email protected]>,
+ Steffen Winterfeldt <[email protected]>, Michael Schroeder <[email protected]>,
+ Ken Wimer <[email protected]>.
+
+Fbsplash, splash protocol design, current implementation & docs by:
+ Michael Januszewski <[email protected]>
+
On Dinsdag 08 M?rz 2005 03:17, Michal Januszewski wrote:
> +It's possible to set path to the splash helper by writing it to
> +/proc/sys/kernel/fbsplash.
It should probably just use its own hotplug agent instead of calling
the helper directly.
> +Splash protocol v1 specified an additional 'fbsplash mode' after the
> +framebuffer number. Splash protocol v1 is deprecated and should not be used.
When you're submitting a patch for inclusion, the interface should really
be stable. I'd completely drop all references to the old version and only
support one interface here. Moreover, you should not do versioned interfaces
unless you expect incompatible changes in future releases. As long as you
still do, that is a sign that the patch is not ready for inclusion.
> +Splash protocol v2 specifies the following commands:
> +
> +getpic
> +------
> + The kernel issues this command to request image data. It's up to the userspace
> + helper to find a background image appropriate for the specified theme and the
> + current resolution. The userspace helper should respond by issuing the
> + FBIOSPLASH_SETPIC ioctl.
> +
> +init
> +----
> + The kernel issues this command after the fbsplash device is created and
> + the fbsplash interface is initialized. Upon receiving 'init', the userspace
> + helper should parse the kernel command line (/proc/cmdline) or otherwise
> + decide whether fbsplash is to be activated.
> +
> + To activate fbsplash on the first console the helper should issue the
> + FBIOSPLASH_SETCFG, FBIOSPLASH_SETPIC and FBIOSPLASH_SETSTATE commands,
> + in the above-mentioned order.
> +
> + 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?
> +modechange
> +----------
> + The kernel issues this command on a mode change. The helper's response should
> + be similar to the response to the 'init' command. Note that this time the
> + console sem is held and all ioctls must be performed with origin set to
> + FB_SPLASH_IO_ORIG_KERNEL.
>
> +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?
> +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 ;-).
> +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.
> +Definition of struct fb_splash_iowrapper can be found in linux/fb.h.
> +The fields in this struct have the following meaning:
> +
> +vc:
> +Virtual console number.
This should not be needed. I notice you are creating your own miscdevice
for passing ioctl commands. That seems a little backwards since there
already is a character device for the frame buffer. Can't you simply
add your ioctl commands there?
> +origin:
> +Specifies if the ioctl is performed as a response to a kernel request. The
> +splash helper should set this field to FB_SPLASH_IO_ORIG_KERNEL, userspace
> +programs should set it to FB_SPLASH_IO_ORIG_USER. This field is necessary to
> +avoid console semaphore deadlocks.
As I mentioned, this means there is thinko in your locking scheme.
> +data:
> +Pointer to a data structure appropriate for the performed ioctl. Type of
> +the data struct is specified in the ioctls description.
This is very wrong. Using ioctl() for anything is bad enough because it
contains an indirection from a system call. Here, you add yet another
indirection. Instead of this, you should at least have a single data
structure for each ioctl number, without using pointers to other structures.
Arnd <><
Arnd Bergmann wrote:
> Nothing about the init command seems really necessary. Why not just do
> that stuff from an /sbin/init script?
I'm not a kernel hacker by any definition, but I'm pretty sure its neccasery
because we want it to be done before /sbin/init is ran, AKA hide the kernel
messages :)
--
[Name ] :: [Matan I. Peled ]
[Location ] :: [Israel ]
[Public Key] :: [0xD6F42CA5 ]
[Keyserver ] :: [keyserver.kjsl.com]
encrypted/signed plain text preferred
On Tue, 8 Mar 2005 04:18:07 +0100, Arnd Bergmann <[email protected]> wrote:
> On Dinsdag 08 M?rz 2005 03:17, Michal Januszewski wrote:
>
> > +It's possible to set path to the splash helper by writing it to
> > +/proc/sys/kernel/fbsplash.
>
> It should probably just use its own hotplug agent instead of calling
> the helper directly.
Framebuffer is already generating hotplug add/remove messages.
HOTPLUG_TIME='Mon Feb 28 02:15:25 EST 2005'
PHYSDEVPATH=/devices/pci0000:00/0000:00:01.0/0000:01:00.0
SUBSYSTEM=graphics
HOTPLUG_ARGS=
DEVPATH=/class/graphics/fb0
HOTPLUG_TYPE=graphics
ACTION=add
PHYSDEVDRIVER=radeonfb
DEBUG=yes
PHYSDEVBUS=pci
SEQNUM=537
This event can be caught in early userspace. The helper app will need
to live on initramfs/initrd and be small. Statically link it to klibc.
You can use the modular fb patch that has been recently posted (just
about to go in -mm) to make hotplug fbdev debugging much simpler.
Alternatively you might want to look at how request_firmware works.
You could easily load the picture as if it were firmware. I'm looking
at modifying request_firmware to be more general right now.
A direct call to call_usermodehelper() requires you to build a path
and app name into the driver.
--
Jon Smirl
[email protected]
On Monday, March 7, 2005 9:00 pm, Matan Peled wrote:
> Arnd Bergmann wrote:
> > Nothing about the init command seems really necessary. Why not just do
> > that stuff from an /sbin/init script?
>
> I'm not a kernel hacker by any definition, but I'm pretty sure its
> neccasery because we want it to be done before /sbin/init is ran, AKA hide
> the kernel messages :)
You can already hide the kernel messages by booting with the 'quiet' option.
It works pretty well and it seems like this stuff should be in userspace
anyway (console support in general that is).
Jesse
On Tue, Mar 08, 2005 at 04:18:07AM +0100, Arnd Bergmann wrote:
> 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?
> > +Splash protocol v1 specified an additional 'fbsplash mode' after the
> > +framebuffer number. Splash protocol v1 is deprecated and should not be used.
> When you're submitting a patch for inclusion, the interface should really
> be stable. I'd completely drop all references to the old version and only
> support one interface here. Moreover, you should not do versioned interfaces
The old one is already not supported. I mentioned v1 in the docs only so
that it would be clear where do the '2' in the helper's command line
arguments come from.
> unless you expect incompatible changes in future releases. As long as you
> still do, that is a sign that the patch is not ready for inclusion.
I don't do now, but who knows when changes will be made to fbcon in the
future? There's a possibility that things will change enough to justify
a new splash protocol.
On the other hand, if the hotplug way of calling userspace were to be
adopted in fbsplash, the whole splash interface with the versioning
scheme could be dropped, which, I guess, would be a good thing.
> > + 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.
I'm currently using a setup that is a combination of both the 'quiet'
option and the 'init' command. I'm using the splash helper to switch to
a selected tty and display a full-screen image, while keeping the tty
in KD_TEXT. This looks pretty nice and lets me see the initial bitmap
for ca. 5 secs.
> > +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.
To make sure no one uses fbsplash for malicious purposes, the
/dev/fbsplash device is used, which is kept root-writable only.
> > +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 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.
> > +Definition of struct fb_splash_iowrapper can be found in linux/fb.h.
> > +The fields in this struct have the following meaning:
> > +
> > +vc:
> > +Virtual console number.
> This should not be needed. I notice you are creating your own miscdevice
> for passing ioctl commands. That seems a little backwards since there
> already is a character device for the frame buffer. Can't you simply
> add your ioctl commands there?
I can, but:
- there are the previously-mentioned security reasons -- the user might
wish to make the fb device writable to some less trusted people, while
the fbsplash device should be kept protected,
- fbsplash calls aren't really related to a particular framebuffer
device, they are related to a tty. 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).
> > +origin:
> > +Specifies if the ioctl is performed as a response to a kernel request. The
> > +splash helper should set this field to FB_SPLASH_IO_ORIG_KERNEL, userspace
> > +programs should set it to FB_SPLASH_IO_ORIG_USER. This field is necessary to
> > +avoid console semaphore deadlocks.
> As I mentioned, this means there is thinko in your locking scheme.
It's not my locking scheme, it's the scheme that is currently used in
the whole console subsystem. If someone can find a sane way to correct
the problem and avoid the whole FB_SPLASH_IO_ORIG* thing, I would be
very heppy to update fbsplash's code.
> > +data:
> > +Pointer to a data structure appropriate for the performed ioctl. Type of
> > +the data struct is specified in the ioctls description.
> This is very wrong. Using ioctl() for anything is bad enough because it
> contains an indirection from a system call. Here, you add yet another
> indirection. Instead of this, you should at least have a single data
> structure for each ioctl number, without using pointers to other structures.
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.
Live long and prosper.
--
Michal 'Spock' Januszewski Gentoo Linux Developer
cell: +48504917690 http://dev.gentoo.org/~spock/
JID: [email protected] freenode: #gentoo-dev, #gentoo-pl
On Dinsdag 08 März 2005 23:37, Michal Januszewski wrote:
> On Tue, Mar 08, 2005 at 04:18:07AM +0100, Arnd Bergmann wrote:
>
> > 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.
> > > + 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.
> > > +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.
> > > +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.
> > > +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 <><
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.
> > 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 :)
> > > 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.
> > 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.
> > 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: [email protected] freenode: #gentoo-dev, #gentoo-pl
On Middeweken 09 März 2005 03:05, Michal Januszewski wrote:
> On Wed, Mar 09, 2005 at 01:17:11AM +0100, Arnd Bergmann wrote:
> 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.
Ok, I now saw that you call call_usermodehelper with wait==1. Why is that
necessary? If you don't wait there, you don't need any hacks around the
console semaphore, because the helper will simply wait for change_console
to finish.
> 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..
Right. I don't think that sysfs is the right interface for this problem.
For a short moment I had the idea that you could write an escape sequence
to the tty device, but that would race against the regular output.
An ioctl on the tty device is probably the best you can do here, but
if that's not possible, creating a misc device in order to do ioctl on it
is a rather ugly hack.
Arnd <><
On Tue, 8 Mar 2005 23:37:29 +0100, Michal Januszewski <[email protected]> wrote:
> On Tue, Mar 08, 2005 at 04:18:07AM +0100, Arnd Bergmann wrote:
>
> > 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
framebuffer already has a class registered. check out /sys/class/grpahics.
You should be able to just call request_firmware and have it download
your image whenever you need it. It doesn't have to be firmware,
request_firmware will download anything.
--
Jon Smirl
[email protected]
On Middeweken 09 M?rz 2005 17:54, Jon Smirl wrote:
> framebuffer already has a class registered. check out /sys/class/grpahics.
>
> You should be able to just call request_firmware and have it download
> your image whenever you need it. It doesn't have to be firmware,
> request_firmware will download anything.
Agreed, request_firmware() would be a really nice way to simplify the interface.
It might be more practical to use the /sys/class/tty/tty* object instead of
the /sys/class/graphics/fb* one, but I don't know enough about the console
subsystem to tell for sure.
Arnd <><
On Wed, Mar 09, 2005 at 04:40:09PM +0100, Arnd Bergmann wrote:
> Ok, I now saw that you call call_usermodehelper with wait==1. Why is that
> necessary? If you don't wait there, you don't need any hacks around the
> console semaphore, because the helper will simply wait for change_console
> to finish.
That could work, assuming that:
a) the helper can always finish unpacking the image before
change_console releases the semaphore
b) the console is not redrawn at one point before change_console is
finished (I'm sorry, I don't have time to check that right now)
If one of this assumptions is wrong, we would end up with first
displaying the wrong background picture and then quickly painting
the new one (I doubt this would make a good impression on the user..).
> > 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..
>
> Right. I don't think that sysfs is the right interface for this problem.
> For a short moment I had the idea that you could write an escape sequence
> to the tty device, but that would race against the regular output.
> An ioctl on the tty device is probably the best you can do here, but
> if that's not possible, creating a misc device in order to do ioctl on it
> is a rather ugly hack.
Agreed, that might not be the nicest thing to do, but it just seems
there isn't much of a choice in this situation.
I'm going to be away for the next 10 days or so. When I get back I'll
start testing some of the proposed changes. Maybe by using things like
firmware_request it would at least be possible to make the splash
interface a little simpler.
Live long and prosper.
--
Michal 'Spock' Januszewski Gentoo Linux Developer
cell: +48504917690 http://dev.gentoo.org/~spock/
JID: [email protected] freenode: #gentoo-dev, #gentoo-pl
Hi!
> +The userspace helper
> +--------------------
> +
> +The userspace splash helper (by default: /sbin/splash_helper) is called by the
> +kernel whenever an important event occurs and the kernel needs some kind of
> +job to be carried out. Important events include console switches and graphic
> +mode switches (the kernel requests background images and configuration
> +parameters for the current console). The splash helper must be accessible at
> +all times. If it's not, fbsplash will be switched off automatically.
Ugh, is not calling userspace when switching consoles deadlock-prone?
What if that helper tries to do printf()?
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
>
> 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.
It should be totally async witout any sem. there is no guarantee that
userspace is even available at the time you do the call.