On Saturday 30 December 2006 00:41, Jeff Dike wrote:
> Clean up the console driver locking. There are various problems here,
> including sleeping under a spinlock and spinlock recursion, some of
> which are fixed here. This patch deals with the locking involved with
> opens and closes. The problem is that an mconsole request to change a
> console's configuration can race with an open. Changing a
> configuration should only be done when a console isn't opened. Also,
> an open must be looking at a stable configuration. In addition, a get
> configuration request must observe the same locking since it must also
> see a stable configuration. With the old locking, it was possible for
> this to hang indefinitely in some cases because open would block for a
> long time waiting for a connection from the host while holding the
> lock needed by the mconsole request.
>
> As explained in the long comment, this is fixed by adding a spinlock
> for the use count and configuration and a mutex for the actual open
> and close.
>
> Signed-off-by: Jeff Dike <[email protected]>
> +
> int line_open(struct line *lines, struct tty_struct *tty)
> {
> - struct line *line;
> + struct line *line = &lines[tty->index];
> int err = -ENODEV;
>
> - line = &lines[tty->index];
> - tty->driver_data = line;
> + spin_lock(&line->count_lock);
> + if(!line->valid)
> + goto out_unlock;
> +
> + err = 0;
> + if(tty->count > 1)
> + goto out_unlock;
>
> - /* The IRQ which takes this lock is not yet enabled and won't be run
> - * before the end, so we don't need to use spin_lock_irq.*/
> - spin_lock(&line->lock);
> + mutex_lock(&line->open_mutex);
> + spin_unlock(&line->count_lock);
This is an obnoxious thing to do unless you specifically prove otherwise. You
cannot take a mutex (and possibly sleep) while holding a spinlock.
You must have either:
+ spin_unlock(&line->count_lock);
+ mutex_lock(&line->open_mutex);
or take count_lock inside open_mutex (which looks like being correct here).
In the first solution, you can create a OPENING flag (via a state variable),
and add the rule that (unlike the count) nobody but the original setter is
allowed to change it, and that who finds it set (say a concurrent open) must
return without touching it.
The state diagram is like:
CLOSED -> OPENING -> OPEN
(only the function which triggered the transition from CLOSED to OPENING can
trigger the transition from OPENING to OPEN). It can probably be simplified
to OPENING <-> ! OPENING.
--
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale!
http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
