* (no subject)
@ 2012-02-12 0:21 Richard Weinberger
2012-02-12 0:24 ` [PATCH] um: Use tty_port Richard Weinberger
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Richard Weinberger @ 2012-02-12 0:21 UTC (permalink / raw)
To: linux-kernel
Cc: user-mode-linux-devel, viro, akpm, alan, gregkh, Richard Weinberger
Can you please review this patch?
Thanks,
//richard
---
>From d8f5e7953def150bcc1e6a39dbbe589f1c68bcbd Mon Sep 17 00:00:00 2001
From: Richard Weinberger <richard@nod.at>
Date: Sun, 12 Feb 2012 01:12:49 +0100
Subject: [PATCH] um: Use tty_port
UML's line driver has to use tty_port.
Signed-off-by: Richard Weinberger <richard@nod.at>
---
arch/um/drivers/line.c | 212 +++++++++++---------------------------
arch/um/drivers/line.h | 13 ++-
arch/um/drivers/ssl.c | 16 +++-
arch/um/drivers/stdio_console.c | 14 ++-
4 files changed, 94 insertions(+), 161 deletions(-)
diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index c1cf220..c789748 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -19,19 +19,29 @@ static irqreturn_t line_interrupt(int irq, void *data)
{
struct chan *chan = data;
struct line *line = chan->line;
+ struct tty_struct *tty;
+
+ if (line) {
+ tty = tty_port_tty_get(&line->port);
+ chan_interrupt(&line->chan_list, &line->task, tty, irq);
+ tty_kref_put(tty);
+ }
- if (line)
- chan_interrupt(&line->chan_list, &line->task, line->tty, irq);
return IRQ_HANDLED;
}
static void line_timer_cb(struct work_struct *work)
{
struct line *line = container_of(work, struct line, task.work);
+ struct tty_struct *tty;
- if (!line->throttled)
- chan_interrupt(&line->chan_list, &line->task, line->tty,
+ if (!line->throttled) {
+ tty = tty_port_tty_get(&line->port);
+ chan_interrupt(&line->chan_list, &line->task, tty,
line->driver->read_irq);
+
+ tty_kref_put(tty);
+ }
}
/*
@@ -228,92 +238,6 @@ void line_set_termios(struct tty_struct *tty, struct ktermios * old)
/* nothing */
}
-static const struct {
- int cmd;
- char *level;
- char *name;
-} tty_ioctls[] = {
- /* don't print these, they flood the log ... */
- { TCGETS, NULL, "TCGETS" },
- { TCSETS, NULL, "TCSETS" },
- { TCSETSW, NULL, "TCSETSW" },
- { TCFLSH, NULL, "TCFLSH" },
- { TCSBRK, NULL, "TCSBRK" },
-
- /* general tty stuff */
- { TCSETSF, KERN_DEBUG, "TCSETSF" },
- { TCGETA, KERN_DEBUG, "TCGETA" },
- { TIOCMGET, KERN_DEBUG, "TIOCMGET" },
- { TCSBRKP, KERN_DEBUG, "TCSBRKP" },
- { TIOCMSET, KERN_DEBUG, "TIOCMSET" },
-
- /* linux-specific ones */
- { TIOCLINUX, KERN_INFO, "TIOCLINUX" },
- { KDGKBMODE, KERN_INFO, "KDGKBMODE" },
- { KDGKBTYPE, KERN_INFO, "KDGKBTYPE" },
- { KDSIGACCEPT, KERN_INFO, "KDSIGACCEPT" },
-};
-
-int line_ioctl(struct tty_struct *tty, unsigned int cmd,
- unsigned long arg)
-{
- int ret;
- int i;
-
- ret = 0;
- switch(cmd) {
-#ifdef TIOCGETP
- case TIOCGETP:
- case TIOCSETP:
- case TIOCSETN:
-#endif
-#ifdef TIOCGETC
- case TIOCGETC:
- case TIOCSETC:
-#endif
-#ifdef TIOCGLTC
- case TIOCGLTC:
- case TIOCSLTC:
-#endif
- /* Note: these are out of date as we now have TCGETS2 etc but this
- whole lot should probably go away */
- case TCGETS:
- case TCSETSF:
- case TCSETSW:
- case TCSETS:
- case TCGETA:
- case TCSETAF:
- case TCSETAW:
- case TCSETA:
- case TCXONC:
- case TCFLSH:
- case TIOCOUTQ:
- case TIOCINQ:
- case TIOCGLCKTRMIOS:
- case TIOCSLCKTRMIOS:
- case TIOCPKT:
- case TIOCGSOFTCAR:
- case TIOCSSOFTCAR:
- return -ENOIOCTLCMD;
-#if 0
- case TCwhatever:
- /* do something */
- break;
-#endif
- default:
- for (i = 0; i < ARRAY_SIZE(tty_ioctls); i++)
- if (cmd == tty_ioctls[i].cmd)
- break;
- if (i == ARRAY_SIZE(tty_ioctls)) {
- printk(KERN_ERR "%s: %s: unknown ioctl: 0x%x\n",
- __func__, tty->name, cmd);
- }
- ret = -ENOIOCTLCMD;
- break;
- }
- return ret;
-}
-
void line_throttle(struct tty_struct *tty)
{
struct line *line = tty->driver_data;
@@ -343,7 +267,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
{
struct chan *chan = data;
struct line *line = chan->line;
- struct tty_struct *tty = line->tty;
+ struct tty_struct *tty = tty_port_tty_get(&line->port);
int err;
/*
@@ -354,6 +278,9 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
spin_lock(&line->lock);
err = flush_buffer(line);
if (err == 0) {
+ tty_kref_put(tty);
+
+ spin_unlock(&line->lock);
return IRQ_NONE;
} else if (err < 0) {
line->head = line->buffer;
@@ -365,9 +292,12 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
return IRQ_NONE;
tty_wakeup(tty);
+ tty_kref_put(tty);
return IRQ_HANDLED;
}
+static const struct tty_port_operations line_port_ops;
+
int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
{
const struct line_driver *driver = line->driver;
@@ -404,27 +334,27 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
* first open or last close. Otherwise, open and close just return.
*/
-int line_open(struct line *lines, struct tty_struct *tty)
+int line_open(struct tty_struct *tty, struct file *filp)
{
- struct line *line = &lines[tty->index];
- int err = -ENODEV;
+ struct line *line = tty->driver_data;
+ return tty_port_open(&line->port, tty, filp);
+}
- spin_lock(&line->count_lock);
- if (!line->valid)
- goto out_unlock;
+int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
+{
+ int ret = tty_init_termios(tty);
- err = 0;
- if (line->count++)
- goto out_unlock;
+ if (ret)
+ return ret;
- BUG_ON(tty->driver_data);
+ tty_driver_kref_get(driver);
+ tty->count++;
tty->driver_data = line;
- line->tty = tty;
+ driver->ttys[tty->index] = tty;
- spin_unlock(&line->count_lock);
- err = enable_chan(line);
- if (err) /* line_close() will be called by our caller */
- return err;
+ ret = enable_chan(line);
+ if (ret)
+ return ret;
INIT_DELAYED_WORK(&line->task, line_timer_cb);
@@ -437,48 +367,36 @@ int line_open(struct line *lines, struct tty_struct *tty)
&tty->winsize.ws_col);
return 0;
-
-out_unlock:
- spin_unlock(&line->count_lock);
- return err;
}
static void unregister_winch(struct tty_struct *tty);
-void line_close(struct tty_struct *tty, struct file * filp)
+void line_cleanup(struct tty_struct *tty)
{
struct line *line = tty->driver_data;
- /*
- * If line_open fails (and tty->driver_data is never set),
- * tty_open will call line_close. So just return in this case.
- */
- if (line == NULL)
- return;
-
- /* We ignore the error anyway! */
- flush_buffer(line);
-
- spin_lock(&line->count_lock);
- BUG_ON(!line->valid);
-
- if (--line->count)
- goto out_unlock;
-
- line->tty = NULL;
- tty->driver_data = NULL;
-
- spin_unlock(&line->count_lock);
-
if (line->sigio) {
unregister_winch(tty);
line->sigio = 0;
}
- return;
+ tty->driver_data = NULL;
+}
+
+void line_close(struct tty_struct *tty, struct file * filp)
+{
+ struct line *line = tty->driver_data;
+
+ if (!line)
+ return;
+
+ tty_port_close(&line->port, tty, filp);
+}
-out_unlock:
- spin_unlock(&line->count_lock);
+void line_hangup(struct tty_struct *tty)
+{
+ struct line *line = tty->driver_data;
+ tty_port_hangup(&line->port);
}
void close_lines(struct line *lines, int nlines)
@@ -495,13 +413,6 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
struct line *line = &lines[n];
int err = -EINVAL;
- spin_lock(&line->count_lock);
-
- if (line->count) {
- *error_out = "Device is already open";
- goto out;
- }
-
if (line->init_pri <= init_prio) {
line->init_pri = init_prio;
if (!strcmp(init, "none"))
@@ -512,8 +423,7 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
}
}
err = 0;
-out:
- spin_unlock(&line->count_lock);
+
return err;
}
@@ -598,6 +508,7 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
struct line *line;
char *end;
int dev, n = 0;
+ struct tty_struct *tty;
dev = simple_strtoul(name, &end, 0);
if ((*end != '\0') || (end == name)) {
@@ -612,13 +523,15 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
line = &lines[dev];
- spin_lock(&line->count_lock);
+ tty = tty_port_tty_get(&line->port);
+
if (!line->valid)
CONFIG_CHUNK(str, size, n, "none", 1);
- else if (line->tty == NULL)
+ else if (tty == NULL)
CONFIG_CHUNK(str, size, n, line->init_str, 1);
else n = chan_config_string(&line->chan_list, str, size, error_out);
- spin_unlock(&line->count_lock);
+
+ tty_kref_put(tty);
return n;
}
@@ -678,8 +591,8 @@ struct tty_driver *register_lines(struct line_driver *line_driver,
}
for(i = 0; i < nlines; i++) {
- if (!lines[i].valid)
- tty_unregister_device(driver, i);
+ tty_port_init(&lines[i].port);
+ lines[i].port.ops = &line_port_ops;
}
mconsole_register_dev(&line_driver->mc);
@@ -805,7 +718,6 @@ void register_winch_irq(int fd, int tty_fd, int pid, struct tty_struct *tty,
.pid = pid,
.tty = tty,
.stack = stack });
-
if (um_request_irq(WINCH_IRQ, fd, IRQ_READ, winch_interrupt,
IRQF_DISABLED | IRQF_SHARED | IRQF_SAMPLE_RANDOM,
"winch", winch) < 0) {
diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h
index 63df3ca..54adfc6 100644
--- a/arch/um/drivers/line.h
+++ b/arch/um/drivers/line.h
@@ -31,9 +31,8 @@ struct line_driver {
};
struct line {
- struct tty_struct *tty;
- spinlock_t count_lock;
- unsigned long count;
+ struct tty_port port;
+
int valid;
char *init_str;
@@ -59,15 +58,17 @@ struct line {
};
#define LINE_INIT(str, d) \
- { .count_lock = __SPIN_LOCK_UNLOCKED((str).count_lock), \
- .init_str = str, \
+ { .init_str = str, \
.init_pri = INIT_STATIC, \
.valid = 1, \
.lock = __SPIN_LOCK_UNLOCKED((str).lock), \
.driver = d }
extern void line_close(struct tty_struct *tty, struct file * filp);
-extern int line_open(struct line *lines, struct tty_struct *tty);
+extern int line_open(struct tty_struct *tty, struct file *filp);
+extern int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line);
+extern void line_cleanup(struct tty_struct *tty);
+extern void line_hangup(struct tty_struct *tty);
extern int line_setup(struct line *lines, unsigned int sizeof_lines,
char *init, char **error_out);
extern int line_write(struct tty_struct *tty, const unsigned char *buf,
diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
index 9d8c20a..89e4e75 100644
--- a/arch/um/drivers/ssl.c
+++ b/arch/um/drivers/ssl.c
@@ -92,6 +92,7 @@ static int ssl_remove(int n, char **error_out)
error_out);
}
+#if 0
static int ssl_open(struct tty_struct *tty, struct file *filp)
{
int err = line_open(serial_lines, tty);
@@ -103,7 +104,6 @@ static int ssl_open(struct tty_struct *tty, struct file *filp)
return err;
}
-#if 0
static void ssl_flush_buffer(struct tty_struct *tty)
{
return;
@@ -124,8 +124,16 @@ void ssl_hangup(struct tty_struct *tty)
}
#endif
+static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+ if (tty->index < NR_PORTS)
+ return line_install(driver, tty, &serial_lines[tty->index]);
+ else
+ return -ENODEV;
+}
+
static const struct tty_operations ssl_ops = {
- .open = ssl_open,
+ .open = line_open,
.close = line_close,
.write = line_write,
.put_char = line_put_char,
@@ -134,9 +142,11 @@ static const struct tty_operations ssl_ops = {
.flush_buffer = line_flush_buffer,
.flush_chars = line_flush_chars,
.set_termios = line_set_termios,
- .ioctl = line_ioctl,
.throttle = line_throttle,
.unthrottle = line_unthrottle,
+ .install = ssl_install,
+ .cleanup = line_cleanup,
+ .hangup = line_hangup,
#if 0
.stop = ssl_stop,
.start = ssl_start,
diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
index 088776f..014f3ee 100644
--- a/arch/um/drivers/stdio_console.c
+++ b/arch/um/drivers/stdio_console.c
@@ -97,7 +97,7 @@ static int con_remove(int n, char **error_out)
static int con_open(struct tty_struct *tty, struct file *filp)
{
- int err = line_open(vts, tty);
+ int err = line_open(tty, filp);
if (err)
printk(KERN_ERR "Failed to open console %d, err = %d\n",
tty->index, err);
@@ -105,6 +105,14 @@ static int con_open(struct tty_struct *tty, struct file *filp)
return err;
}
+static int con_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+ if (tty->index < MAX_TTYS)
+ return line_install(driver, tty, &vts[tty->index]);
+ else
+ return -ENODEV;
+}
+
/* Set in an initcall, checked in an exitcall */
static int con_init_done = 0;
@@ -118,9 +126,11 @@ static const struct tty_operations console_ops = {
.flush_buffer = line_flush_buffer,
.flush_chars = line_flush_chars,
.set_termios = line_set_termios,
- .ioctl = line_ioctl,
.throttle = line_throttle,
.unthrottle = line_unthrottle,
+ .cleanup = line_cleanup,
+ .install = con_install,
+ .hangup = line_hangup,
};
static void uml_console_write(struct console *console, const char *string,
--
1.7.7.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] um: Use tty_port
2012-02-12 0:21 Richard Weinberger
@ 2012-02-12 0:24 ` Richard Weinberger
2012-02-12 13:01 ` Jiri Slaby
2012-02-12 0:25 ` your mail Jesper Juhl
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2012-02-12 0:24 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, user-mode-linux-devel, viro, akpm, alan, gregkh
[-- Attachment #1: Type: text/plain, Size: 14570 bytes --]
Whoops, I messed up the subject line.
Sorry!
Am 12.02.2012 01:21, schrieb Richard Weinberger:
> Can you please review this patch?
>
> Thanks,
> //richard
>
> ---
> From d8f5e7953def150bcc1e6a39dbbe589f1c68bcbd Mon Sep 17 00:00:00 2001
> From: Richard Weinberger <richard@nod.at>
> Date: Sun, 12 Feb 2012 01:12:49 +0100
> Subject: [PATCH] um: Use tty_port
>
> UML's line driver has to use tty_port.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> arch/um/drivers/line.c | 212 +++++++++++---------------------------
> arch/um/drivers/line.h | 13 ++-
> arch/um/drivers/ssl.c | 16 +++-
> arch/um/drivers/stdio_console.c | 14 ++-
> 4 files changed, 94 insertions(+), 161 deletions(-)
>
> diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
> index c1cf220..c789748 100644
> --- a/arch/um/drivers/line.c
> +++ b/arch/um/drivers/line.c
> @@ -19,19 +19,29 @@ static irqreturn_t line_interrupt(int irq, void *data)
> {
> struct chan *chan = data;
> struct line *line = chan->line;
> + struct tty_struct *tty;
> +
> + if (line) {
> + tty = tty_port_tty_get(&line->port);
> + chan_interrupt(&line->chan_list, &line->task, tty, irq);
> + tty_kref_put(tty);
> + }
>
> - if (line)
> - chan_interrupt(&line->chan_list, &line->task, line->tty, irq);
> return IRQ_HANDLED;
> }
>
> static void line_timer_cb(struct work_struct *work)
> {
> struct line *line = container_of(work, struct line, task.work);
> + struct tty_struct *tty;
>
> - if (!line->throttled)
> - chan_interrupt(&line->chan_list, &line->task, line->tty,
> + if (!line->throttled) {
> + tty = tty_port_tty_get(&line->port);
> + chan_interrupt(&line->chan_list, &line->task, tty,
> line->driver->read_irq);
> +
> + tty_kref_put(tty);
> + }
> }
>
> /*
> @@ -228,92 +238,6 @@ void line_set_termios(struct tty_struct *tty, struct ktermios * old)
> /* nothing */
> }
>
> -static const struct {
> - int cmd;
> - char *level;
> - char *name;
> -} tty_ioctls[] = {
> - /* don't print these, they flood the log ... */
> - { TCGETS, NULL, "TCGETS" },
> - { TCSETS, NULL, "TCSETS" },
> - { TCSETSW, NULL, "TCSETSW" },
> - { TCFLSH, NULL, "TCFLSH" },
> - { TCSBRK, NULL, "TCSBRK" },
> -
> - /* general tty stuff */
> - { TCSETSF, KERN_DEBUG, "TCSETSF" },
> - { TCGETA, KERN_DEBUG, "TCGETA" },
> - { TIOCMGET, KERN_DEBUG, "TIOCMGET" },
> - { TCSBRKP, KERN_DEBUG, "TCSBRKP" },
> - { TIOCMSET, KERN_DEBUG, "TIOCMSET" },
> -
> - /* linux-specific ones */
> - { TIOCLINUX, KERN_INFO, "TIOCLINUX" },
> - { KDGKBMODE, KERN_INFO, "KDGKBMODE" },
> - { KDGKBTYPE, KERN_INFO, "KDGKBTYPE" },
> - { KDSIGACCEPT, KERN_INFO, "KDSIGACCEPT" },
> -};
> -
> -int line_ioctl(struct tty_struct *tty, unsigned int cmd,
> - unsigned long arg)
> -{
> - int ret;
> - int i;
> -
> - ret = 0;
> - switch(cmd) {
> -#ifdef TIOCGETP
> - case TIOCGETP:
> - case TIOCSETP:
> - case TIOCSETN:
> -#endif
> -#ifdef TIOCGETC
> - case TIOCGETC:
> - case TIOCSETC:
> -#endif
> -#ifdef TIOCGLTC
> - case TIOCGLTC:
> - case TIOCSLTC:
> -#endif
> - /* Note: these are out of date as we now have TCGETS2 etc but this
> - whole lot should probably go away */
> - case TCGETS:
> - case TCSETSF:
> - case TCSETSW:
> - case TCSETS:
> - case TCGETA:
> - case TCSETAF:
> - case TCSETAW:
> - case TCSETA:
> - case TCXONC:
> - case TCFLSH:
> - case TIOCOUTQ:
> - case TIOCINQ:
> - case TIOCGLCKTRMIOS:
> - case TIOCSLCKTRMIOS:
> - case TIOCPKT:
> - case TIOCGSOFTCAR:
> - case TIOCSSOFTCAR:
> - return -ENOIOCTLCMD;
> -#if 0
> - case TCwhatever:
> - /* do something */
> - break;
> -#endif
> - default:
> - for (i = 0; i < ARRAY_SIZE(tty_ioctls); i++)
> - if (cmd == tty_ioctls[i].cmd)
> - break;
> - if (i == ARRAY_SIZE(tty_ioctls)) {
> - printk(KERN_ERR "%s: %s: unknown ioctl: 0x%x\n",
> - __func__, tty->name, cmd);
> - }
> - ret = -ENOIOCTLCMD;
> - break;
> - }
> - return ret;
> -}
> -
> void line_throttle(struct tty_struct *tty)
> {
> struct line *line = tty->driver_data;
> @@ -343,7 +267,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> {
> struct chan *chan = data;
> struct line *line = chan->line;
> - struct tty_struct *tty = line->tty;
> + struct tty_struct *tty = tty_port_tty_get(&line->port);
> int err;
>
> /*
> @@ -354,6 +278,9 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> spin_lock(&line->lock);
> err = flush_buffer(line);
> if (err == 0) {
> + tty_kref_put(tty);
> +
> + spin_unlock(&line->lock);
> return IRQ_NONE;
> } else if (err < 0) {
> line->head = line->buffer;
> @@ -365,9 +292,12 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> return IRQ_NONE;
>
> tty_wakeup(tty);
> + tty_kref_put(tty);
> return IRQ_HANDLED;
> }
>
> +static const struct tty_port_operations line_port_ops;
> +
> int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
> {
> const struct line_driver *driver = line->driver;
> @@ -404,27 +334,27 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
> * first open or last close. Otherwise, open and close just return.
> */
>
> -int line_open(struct line *lines, struct tty_struct *tty)
> +int line_open(struct tty_struct *tty, struct file *filp)
> {
> - struct line *line = &lines[tty->index];
> - int err = -ENODEV;
> + struct line *line = tty->driver_data;
> + return tty_port_open(&line->port, tty, filp);
> +}
>
> - spin_lock(&line->count_lock);
> - if (!line->valid)
> - goto out_unlock;
> +int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
> +{
> + int ret = tty_init_termios(tty);
>
> - err = 0;
> - if (line->count++)
> - goto out_unlock;
> + if (ret)
> + return ret;
>
> - BUG_ON(tty->driver_data);
> + tty_driver_kref_get(driver);
> + tty->count++;
> tty->driver_data = line;
> - line->tty = tty;
> + driver->ttys[tty->index] = tty;
>
> - spin_unlock(&line->count_lock);
> - err = enable_chan(line);
> - if (err) /* line_close() will be called by our caller */
> - return err;
> + ret = enable_chan(line);
> + if (ret)
> + return ret;
>
> INIT_DELAYED_WORK(&line->task, line_timer_cb);
>
> @@ -437,48 +367,36 @@ int line_open(struct line *lines, struct tty_struct *tty)
> &tty->winsize.ws_col);
>
> return 0;
> -
> -out_unlock:
> - spin_unlock(&line->count_lock);
> - return err;
> }
>
> static void unregister_winch(struct tty_struct *tty);
>
> -void line_close(struct tty_struct *tty, struct file * filp)
> +void line_cleanup(struct tty_struct *tty)
> {
> struct line *line = tty->driver_data;
>
> - /*
> - * If line_open fails (and tty->driver_data is never set),
> - * tty_open will call line_close. So just return in this case.
> - */
> - if (line == NULL)
> - return;
> -
> - /* We ignore the error anyway! */
> - flush_buffer(line);
> -
> - spin_lock(&line->count_lock);
> - BUG_ON(!line->valid);
> -
> - if (--line->count)
> - goto out_unlock;
> -
> - line->tty = NULL;
> - tty->driver_data = NULL;
> -
> - spin_unlock(&line->count_lock);
> -
> if (line->sigio) {
> unregister_winch(tty);
> line->sigio = 0;
> }
>
> - return;
> + tty->driver_data = NULL;
> +}
> +
> +void line_close(struct tty_struct *tty, struct file * filp)
> +{
> + struct line *line = tty->driver_data;
> +
> + if (!line)
> + return;
> +
> + tty_port_close(&line->port, tty, filp);
> +}
>
> -out_unlock:
> - spin_unlock(&line->count_lock);
> +void line_hangup(struct tty_struct *tty)
> +{
> + struct line *line = tty->driver_data;
> + tty_port_hangup(&line->port);
> }
>
> void close_lines(struct line *lines, int nlines)
> @@ -495,13 +413,6 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
> struct line *line = &lines[n];
> int err = -EINVAL;
>
> - spin_lock(&line->count_lock);
> -
> - if (line->count) {
> - *error_out = "Device is already open";
> - goto out;
> - }
> -
> if (line->init_pri <= init_prio) {
> line->init_pri = init_prio;
> if (!strcmp(init, "none"))
> @@ -512,8 +423,7 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
> }
> }
> err = 0;
> -out:
> - spin_unlock(&line->count_lock);
> +
> return err;
> }
>
> @@ -598,6 +508,7 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
> struct line *line;
> char *end;
> int dev, n = 0;
> + struct tty_struct *tty;
>
> dev = simple_strtoul(name, &end, 0);
> if ((*end != '\0') || (end == name)) {
> @@ -612,13 +523,15 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
>
> line = &lines[dev];
>
> - spin_lock(&line->count_lock);
> + tty = tty_port_tty_get(&line->port);
> +
> if (!line->valid)
> CONFIG_CHUNK(str, size, n, "none", 1);
> - else if (line->tty == NULL)
> + else if (tty == NULL)
> CONFIG_CHUNK(str, size, n, line->init_str, 1);
> else n = chan_config_string(&line->chan_list, str, size, error_out);
> - spin_unlock(&line->count_lock);
> +
> + tty_kref_put(tty);
>
> return n;
> }
> @@ -678,8 +591,8 @@ struct tty_driver *register_lines(struct line_driver *line_driver,
> }
>
> for(i = 0; i < nlines; i++) {
> - if (!lines[i].valid)
> - tty_unregister_device(driver, i);
> + tty_port_init(&lines[i].port);
> + lines[i].port.ops = &line_port_ops;
> }
>
> mconsole_register_dev(&line_driver->mc);
> @@ -805,7 +718,6 @@ void register_winch_irq(int fd, int tty_fd, int pid, struct tty_struct *tty,
> .pid = pid,
> .tty = tty,
> .stack = stack });
> -
> if (um_request_irq(WINCH_IRQ, fd, IRQ_READ, winch_interrupt,
> IRQF_DISABLED | IRQF_SHARED | IRQF_SAMPLE_RANDOM,
> "winch", winch) < 0) {
> diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h
> index 63df3ca..54adfc6 100644
> --- a/arch/um/drivers/line.h
> +++ b/arch/um/drivers/line.h
> @@ -31,9 +31,8 @@ struct line_driver {
> };
>
> struct line {
> - struct tty_struct *tty;
> - spinlock_t count_lock;
> - unsigned long count;
> + struct tty_port port;
> +
> int valid;
>
> char *init_str;
> @@ -59,15 +58,17 @@ struct line {
> };
>
> #define LINE_INIT(str, d) \
> - { .count_lock = __SPIN_LOCK_UNLOCKED((str).count_lock), \
> - .init_str = str, \
> + { .init_str = str, \
> .init_pri = INIT_STATIC, \
> .valid = 1, \
> .lock = __SPIN_LOCK_UNLOCKED((str).lock), \
> .driver = d }
>
> extern void line_close(struct tty_struct *tty, struct file * filp);
> -extern int line_open(struct line *lines, struct tty_struct *tty);
> +extern int line_open(struct tty_struct *tty, struct file *filp);
> +extern int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line);
> +extern void line_cleanup(struct tty_struct *tty);
> +extern void line_hangup(struct tty_struct *tty);
> extern int line_setup(struct line *lines, unsigned int sizeof_lines,
> char *init, char **error_out);
> extern int line_write(struct tty_struct *tty, const unsigned char *buf,
> diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
> index 9d8c20a..89e4e75 100644
> --- a/arch/um/drivers/ssl.c
> +++ b/arch/um/drivers/ssl.c
> @@ -92,6 +92,7 @@ static int ssl_remove(int n, char **error_out)
> error_out);
> }
>
> +#if 0
> static int ssl_open(struct tty_struct *tty, struct file *filp)
> {
> int err = line_open(serial_lines, tty);
> @@ -103,7 +104,6 @@ static int ssl_open(struct tty_struct *tty, struct file *filp)
> return err;
> }
>
> -#if 0
> static void ssl_flush_buffer(struct tty_struct *tty)
> {
> return;
> @@ -124,8 +124,16 @@ void ssl_hangup(struct tty_struct *tty)
> }
> #endif
>
> +static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + if (tty->index < NR_PORTS)
> + return line_install(driver, tty, &serial_lines[tty->index]);
> + else
> + return -ENODEV;
> +}
> +
> static const struct tty_operations ssl_ops = {
> - .open = ssl_open,
> + .open = line_open,
> .close = line_close,
> .write = line_write,
> .put_char = line_put_char,
> @@ -134,9 +142,11 @@ static const struct tty_operations ssl_ops = {
> .flush_buffer = line_flush_buffer,
> .flush_chars = line_flush_chars,
> .set_termios = line_set_termios,
> - .ioctl = line_ioctl,
> .throttle = line_throttle,
> .unthrottle = line_unthrottle,
> + .install = ssl_install,
> + .cleanup = line_cleanup,
> + .hangup = line_hangup,
> #if 0
> .stop = ssl_stop,
> .start = ssl_start,
> diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
> index 088776f..014f3ee 100644
> --- a/arch/um/drivers/stdio_console.c
> +++ b/arch/um/drivers/stdio_console.c
> @@ -97,7 +97,7 @@ static int con_remove(int n, char **error_out)
>
> static int con_open(struct tty_struct *tty, struct file *filp)
> {
> - int err = line_open(vts, tty);
> + int err = line_open(tty, filp);
> if (err)
> printk(KERN_ERR "Failed to open console %d, err = %d\n",
> tty->index, err);
> @@ -105,6 +105,14 @@ static int con_open(struct tty_struct *tty, struct file *filp)
> return err;
> }
>
> +static int con_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + if (tty->index < MAX_TTYS)
> + return line_install(driver, tty, &vts[tty->index]);
> + else
> + return -ENODEV;
> +}
> +
> /* Set in an initcall, checked in an exitcall */
> static int con_init_done = 0;
>
> @@ -118,9 +126,11 @@ static const struct tty_operations console_ops = {
> .flush_buffer = line_flush_buffer,
> .flush_chars = line_flush_chars,
> .set_termios = line_set_termios,
> - .ioctl = line_ioctl,
> .throttle = line_throttle,
> .unthrottle = line_unthrottle,
> + .cleanup = line_cleanup,
> + .install = con_install,
> + .hangup = line_hangup,
> };
>
> static void uml_console_write(struct console *console, const char *string,
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] um: Use tty_port
2012-02-12 0:24 ` [PATCH] um: Use tty_port Richard Weinberger
@ 2012-02-12 13:01 ` Jiri Slaby
2012-02-12 13:12 ` Richard Weinberger
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2012-02-12 13:01 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, user-mode-linux-devel, viro, akpm, alan, gregkh,
Jiri Slaby
On 02/12/2012 01:24 AM, Richard Weinberger wrote:
>> @@ -228,92 +238,6 @@ void line_set_termios(struct tty_struct *tty, struct ktermios * old)
>> /* nothing */
>> }
>>
>> -static const struct {
>> - int cmd;
>> - char *level;
>> - char *name;
>> -} tty_ioctls[] = {
>> - /* don't print these, they flood the log ... */
>> - { TCGETS, NULL, "TCGETS" },
>> - { TCSETS, NULL, "TCSETS" },
>> - { TCSETSW, NULL, "TCSETSW" },
>> - { TCFLSH, NULL, "TCFLSH" },
>> - { TCSBRK, NULL, "TCSBRK" },
>> -
>> - /* general tty stuff */
>> - { TCSETSF, KERN_DEBUG, "TCSETSF" },
>> - { TCGETA, KERN_DEBUG, "TCGETA" },
>> - { TIOCMGET, KERN_DEBUG, "TIOCMGET" },
>> - { TCSBRKP, KERN_DEBUG, "TCSBRKP" },
>> - { TIOCMSET, KERN_DEBUG, "TIOCMSET" },
>> -
>> - /* linux-specific ones */
>> - { TIOCLINUX, KERN_INFO, "TIOCLINUX" },
>> - { KDGKBMODE, KERN_INFO, "KDGKBMODE" },
>> - { KDGKBTYPE, KERN_INFO, "KDGKBTYPE" },
>> - { KDSIGACCEPT, KERN_INFO, "KDSIGACCEPT" },
>> -};
>> -
>> -int line_ioctl(struct tty_struct *tty, unsigned int cmd,
>> - unsigned long arg)
>> -{
>> - int ret;
>> - int i;
>> -
>> - ret = 0;
>> - switch(cmd) {
>> -#ifdef TIOCGETP
>> - case TIOCGETP:
>> - case TIOCSETP:
>> - case TIOCSETN:
>> -#endif
>> -#ifdef TIOCGETC
>> - case TIOCGETC:
>> - case TIOCSETC:
>> -#endif
>> -#ifdef TIOCGLTC
>> - case TIOCGLTC:
>> - case TIOCSLTC:
>> -#endif
>> - /* Note: these are out of date as we now have TCGETS2 etc but this
>> - whole lot should probably go away */
>> - case TCGETS:
>> - case TCSETSF:
>> - case TCSETSW:
>> - case TCSETS:
>> - case TCGETA:
>> - case TCSETAF:
>> - case TCSETAW:
>> - case TCSETA:
>> - case TCXONC:
>> - case TCFLSH:
>> - case TIOCOUTQ:
>> - case TIOCINQ:
>> - case TIOCGLCKTRMIOS:
>> - case TIOCSLCKTRMIOS:
>> - case TIOCPKT:
>> - case TIOCGSOFTCAR:
>> - case TIOCSSOFTCAR:
>> - return -ENOIOCTLCMD;
>> -#if 0
>> - case TCwhatever:
>> - /* do something */
>> - break;
>> -#endif
>> - default:
>> - for (i = 0; i < ARRAY_SIZE(tty_ioctls); i++)
>> - if (cmd == tty_ioctls[i].cmd)
>> - break;
>> - if (i == ARRAY_SIZE(tty_ioctls)) {
>> - printk(KERN_ERR "%s: %s: unknown ioctl: 0x%x\n",
>> - __func__, tty->name, cmd);
>> - }
>> - ret = -ENOIOCTLCMD;
>> - break;
>> - }
>> - return ret;
>> -}
>> -
>> void line_throttle(struct tty_struct *tty)
>> {
>> struct line *line = tty->driver_data;
>> @@ -343,7 +267,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
>> {
>> struct chan *chan = data;
>> struct line *line = chan->line;
>> - struct tty_struct *tty = line->tty;
>> + struct tty_struct *tty = tty_port_tty_get(&line->port);
>> int err;
>>
>> /*
>> @@ -354,6 +278,9 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
>> spin_lock(&line->lock);
>> err = flush_buffer(line);
>> if (err == 0) {
>> + tty_kref_put(tty);
>> +
>> + spin_unlock(&line->lock);
This and the ioctl change above fullfils the subject of the patch in no
way. Do this fix and the cleanup above separately, please.
>> @@ -404,27 +334,27 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
>> * first open or last close. Otherwise, open and close just return.
>> */
>>
>> -int line_open(struct line *lines, struct tty_struct *tty)
>> +int line_open(struct tty_struct *tty, struct file *filp)
>> {
>> - struct line *line = &lines[tty->index];
>> - int err = -ENODEV;
>> + struct line *line = tty->driver_data;
>> + return tty_port_open(&line->port, tty, filp);
>> +}
>>
>> - spin_lock(&line->count_lock);
>> - if (!line->valid)
>> - goto out_unlock;
>> +int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
As it stands, it should be tty_port->ops->activate, not
tty->ops->install. Yes, you can still set driver_data and check for
multiple opens in install. Then use also tty_standard_install.
>> +{
>> + int ret = tty_init_termios(tty);
>>
>> - err = 0;
>> - if (line->count++)
>> - goto out_unlock;
>> + if (ret)
>> + return ret;
>>
>> - BUG_ON(tty->driver_data);
>> + tty_driver_kref_get(driver);
>> + tty->count++;
>> tty->driver_data = line;
>> - line->tty = tty;
>> + driver->ttys[tty->index] = tty;
>>
>> - spin_unlock(&line->count_lock);
>> - err = enable_chan(line);
>> - if (err) /* line_close() will be called by our caller */
>> - return err;
>> + ret = enable_chan(line);
>> + if (ret)
>> + return ret;
>>
>> INIT_DELAYED_WORK(&line->task, line_timer_cb);
>>
>> @@ -437,48 +367,36 @@ int line_open(struct line *lines, struct tty_struct *tty)
>> &tty->winsize.ws_col);
>>
>> return 0;
>> -
>> -out_unlock:
>> - spin_unlock(&line->count_lock);
>> - return err;
>> }
...
>> +void line_close(struct tty_struct *tty, struct file * filp)
>> +{
>> + struct line *line = tty->driver_data;
>> +
>> + if (!line)
>> + return;
Unless you set tty->driver_data to NULL somewhere, this can never
happen. If you do, why -- this tends to be racy?
>> + tty_port_close(&line->port, tty, filp);
>> +}
...
>> --- a/arch/um/drivers/ssl.c
>> +++ b/arch/um/drivers/ssl.c
>> @@ -92,6 +92,7 @@ static int ssl_remove(int n, char **error_out)
>> error_out);
>> }
>>
>> +#if 0
Hmm, remove unused code instead.
>> static int ssl_open(struct tty_struct *tty, struct file *filp)
>> {
>> int err = line_open(serial_lines, tty);
...
>> @@ -124,8 +124,16 @@ void ssl_hangup(struct tty_struct *tty)
>> }
>> #endif
>>
>> +static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> + if (tty->index < NR_PORTS)
Do you register more than NR_PORTS when allocating tty_driver? If not,
this test is always true. But presumably you won't need this hook anyway.
>> + return line_install(driver, tty, &serial_lines[tty->index]);
>> + else
>> + return -ENODEV;
>> +}
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] um: Use tty_port
2012-02-12 13:01 ` Jiri Slaby
@ 2012-02-12 13:12 ` Richard Weinberger
0 siblings, 0 replies; 11+ messages in thread
From: Richard Weinberger @ 2012-02-12 13:12 UTC (permalink / raw)
To: Jiri Slaby
Cc: linux-kernel, user-mode-linux-devel, viro, akpm, alan, gregkh,
Jiri Slaby
[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]
Am 12.02.2012 14:01, schrieb Jiri Slaby:
> This and the ioctl change above fullfils the subject of the patch in no
> way. Do this fix and the cleanup above separately, please.
Fair point.
>
>>> @@ -404,27 +334,27 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
>>> * first open or last close. Otherwise, open and close just return.
>>> */
>>>
>>> -int line_open(struct line *lines, struct tty_struct *tty)
>>> +int line_open(struct tty_struct *tty, struct file *filp)
>>> {
>>> - struct line *line = &lines[tty->index];
>>> - int err = -ENODEV;
>>> + struct line *line = tty->driver_data;
>>> + return tty_port_open(&line->port, tty, filp);
>>> +}
>>>
>>> - spin_lock(&line->count_lock);
>>> - if (!line->valid)
>>> - goto out_unlock;
>>> +int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
>
> As it stands, it should be tty_port->ops->activate, not
> tty->ops->install. Yes, you can still set driver_data and check for
> multiple opens in install. Then use also tty_standard_install.
Okay.
>>> +{
>>> + int ret = tty_init_termios(tty);
>>>
>>> - err = 0;
>>> - if (line->count++)
>>> - goto out_unlock;
>>> + if (ret)
>>> + return ret;
>>>
>>> - BUG_ON(tty->driver_data);
>>> + tty_driver_kref_get(driver);
>>> + tty->count++;
>>> tty->driver_data = line;
>>> - line->tty = tty;
>>> + driver->ttys[tty->index] = tty;
>>>
>>> - spin_unlock(&line->count_lock);
>>> - err = enable_chan(line);
>>> - if (err) /* line_close() will be called by our caller */
>>> - return err;
>>> + ret = enable_chan(line);
>>> + if (ret)
>>> + return ret;
>>>
>>> INIT_DELAYED_WORK(&line->task, line_timer_cb);
>>>
>>> @@ -437,48 +367,36 @@ int line_open(struct line *lines, struct tty_struct *tty)
>>> &tty->winsize.ws_col);
>>>
>>> return 0;
>>> -
>>> -out_unlock:
>>> - spin_unlock(&line->count_lock);
>>> - return err;
>>> }
> ...
>>> +void line_close(struct tty_struct *tty, struct file * filp)
>>> +{
>>> + struct line *line = tty->driver_data;
>>> +
>>> + if (!line)
>>> + return;
>
> Unless you set tty->driver_data to NULL somewhere, this can never
> happen. If you do, why -- this tends to be racy?
The old driver set tty->driver_data to NULL.
I guess we can remove it.
>>> + tty_port_close(&line->port, tty, filp);
>>> +}
> ...
>>> --- a/arch/um/drivers/ssl.c
>>> +++ b/arch/um/drivers/ssl.c
>>> @@ -92,6 +92,7 @@ static int ssl_remove(int n, char **error_out)
>>> error_out);
>>> }
>>>
>>> +#if 0
>
> Hmm, remove unused code instead.
Will do.
>>> static int ssl_open(struct tty_struct *tty, struct file *filp)
>>> {
>>> int err = line_open(serial_lines, tty);
> ...
>>> @@ -124,8 +124,16 @@ void ssl_hangup(struct tty_struct *tty)
>>> }
>>> #endif
>>>
>>> +static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
>>> +{
>>> + if (tty->index < NR_PORTS)
>
> Do you register more than NR_PORTS when allocating tty_driver? If not,
> this test is always true. But presumably you won't need this hook anyway.
Okay.
>>> + return line_install(driver, tty, &serial_lines[tty->index]);
>>> + else
>>> + return -ENODEV;
>>> +}
>
> thanks,
Thanks,
//richard
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 0:21 Richard Weinberger
2012-02-12 0:24 ` [PATCH] um: Use tty_port Richard Weinberger
@ 2012-02-12 0:25 ` Jesper Juhl
2012-02-12 1:02 ` Al Viro
2012-02-12 19:11 ` Al Viro
3 siblings, 0 replies; 11+ messages in thread
From: Jesper Juhl @ 2012-02-12 0:25 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, user-mode-linux-devel, viro, akpm, alan, gregkh
On Sun, 12 Feb 2012, Richard Weinberger wrote:
> Can you please review this patch?
>
A subject on the mail along with a description of the patch would make
that a great deal easier...
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 0:21 Richard Weinberger
2012-02-12 0:24 ` [PATCH] um: Use tty_port Richard Weinberger
2012-02-12 0:25 ` your mail Jesper Juhl
@ 2012-02-12 1:02 ` Al Viro
2012-02-12 12:40 ` Jiri Slaby
2012-02-12 19:11 ` Al Viro
3 siblings, 1 reply; 11+ messages in thread
From: Al Viro @ 2012-02-12 1:02 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, user-mode-linux-devel, akpm, alan, gregkh
On Sun, Feb 12, 2012 at 01:21:10AM +0100, Richard Weinberger wrote:
Not a full review by any means, but...
> +++ b/arch/um/drivers/line.c
> @@ -19,19 +19,29 @@ static irqreturn_t line_interrupt(int irq, void *data)
> {
> struct chan *chan = data;
> struct line *line = chan->line;
> + struct tty_struct *tty;
> +
> + if (line) {
> + tty = tty_port_tty_get(&line->port);
> + chan_interrupt(&line->chan_list, &line->task, tty, irq);
> + tty_kref_put(tty);
> + }
>
> - if (line)
> - chan_interrupt(&line->chan_list, &line->task, line->tty, irq);
> return IRQ_HANDLED;
> }
Is tty_kref_put() safe in interrupt? Here it seems to be OK, but in other
callers... More or less at random: drivers/tty/serial/lantiq.c has it
called from lqasc_rx_int(). It seems to be possible to have it end up
calling ->ops->shutdown() and in this case that'd be lqasc_shutdown().
Which does a bunch of free_irq(), including the ->rx_irq, i.e. the one
we have it called from. Alan?
> @@ -495,13 +413,6 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
> struct line *line = &lines[n];
> int err = -EINVAL;
>
> - spin_lock(&line->count_lock);
> -
> - if (line->count) {
> - *error_out = "Device is already open";
> - goto out;
> - }
... and similar in line_open() - just what happens if you try to reconfigure
an opened one?
> @@ -612,13 +523,15 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
>
> line = &lines[dev];
>
> - spin_lock(&line->count_lock);
> + tty = tty_port_tty_get(&line->port);
> +
> if (!line->valid)
> CONFIG_CHUNK(str, size, n, "none", 1);
> - else if (line->tty == NULL)
> + else if (tty == NULL)
> CONFIG_CHUNK(str, size, n, line->init_str, 1);
> else n = chan_config_string(&line->chan_list, str, size, error_out);
> - spin_unlock(&line->count_lock);
> +
> + tty_kref_put(tty);
again, where's the exclusion with config changes?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 1:02 ` Al Viro
@ 2012-02-12 12:40 ` Jiri Slaby
2012-02-12 19:06 ` Al Viro
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2012-02-12 12:40 UTC (permalink / raw)
To: Al Viro
Cc: Richard Weinberger, linux-kernel, user-mode-linux-devel, akpm,
alan, gregkh, Jiri Slaby
On 02/12/2012 02:02 AM, Al Viro wrote:
> On Sun, Feb 12, 2012 at 01:21:10AM +0100, Richard Weinberger wrote:
>> +++ b/arch/um/drivers/line.c
>> @@ -19,19 +19,29 @@ static irqreturn_t line_interrupt(int irq, void *data)
>> {
>> struct chan *chan = data;
>> struct line *line = chan->line;
>> + struct tty_struct *tty;
>> +
>> + if (line) {
>> + tty = tty_port_tty_get(&line->port);
>> + chan_interrupt(&line->chan_list, &line->task, tty, irq);
>> + tty_kref_put(tty);
>> + }
>>
>> - if (line)
>> - chan_interrupt(&line->chan_list, &line->task, line->tty, irq);
>> return IRQ_HANDLED;
>> }
>
> Is tty_kref_put() safe in interrupt? Here it seems to be OK, but in other
> callers... More or less at random: drivers/tty/serial/lantiq.c has it
> called from lqasc_rx_int(). It seems to be possible to have it end up
> calling ->ops->shutdown() and in this case that'd be lqasc_shutdown().
> Which does a bunch of free_irq(), including the ->rx_irq, i.e. the one
> we have it called from. Alan?
I'm not Alan, but will reply anyway. Yes, it is safe (unless the driver
does something tricky). In the driver you mention, this is uart_ops,
called from tty_port_operations' ->shutdown. And that's a different from
tty_operations' ->shutdown.
Yes, there are:
* tty->ops
* tty_port->ops
* uart_port->ops
uart_port->ops->shutdown is supposed to tear down interrupts like in
lantiq.c. It is called from tty_port->ops->shutdown. And that one is
allowed to be called only from user context (tty->ops->close and
tty->ops->hangup).
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 12:40 ` Jiri Slaby
@ 2012-02-12 19:06 ` Al Viro
2012-02-13 9:40 ` Jiri Slaby
0 siblings, 1 reply; 11+ messages in thread
From: Al Viro @ 2012-02-12 19:06 UTC (permalink / raw)
To: Jiri Slaby
Cc: Richard Weinberger, linux-kernel, user-mode-linux-devel, akpm,
alan, gregkh, Jiri Slaby
On Sun, Feb 12, 2012 at 01:40:47PM +0100, Jiri Slaby wrote:
> > Is tty_kref_put() safe in interrupt? Here it seems to be OK, but in other
> > callers... More or less at random: drivers/tty/serial/lantiq.c has it
> > called from lqasc_rx_int(). It seems to be possible to have it end up
> > calling ->ops->shutdown() and in this case that'd be lqasc_shutdown().
> > Which does a bunch of free_irq(), including the ->rx_irq, i.e. the one
> > we have it called from. Alan?
>
> I'm not Alan, but will reply anyway. Yes, it is safe (unless the driver
> does something tricky). In the driver you mention, this is uart_ops,
> called from tty_port_operations' ->shutdown. And that's a different from
> tty_operations' ->shutdown.
>
> Yes, there are:
> * tty->ops
> * tty_port->ops
> * uart_port->ops
>
> uart_port->ops->shutdown is supposed to tear down interrupts like in
> lantiq.c. It is called from tty_port->ops->shutdown. And that one is
> allowed to be called only from user context (tty->ops->close and
> tty->ops->hangup).
Yecchhh... If I'm reading (and grepping) it right, there are only two
non-default instance of tty_operations ->shutdown() - pty and vt ones.
Lovely... And while we are at it, vt instance is definitely not safe
from interrupts - calls console_lock(). Not that it was relevant in
this case...
It's probably too late in this case, but I would've called that method
->sync_cleanup(). Assuming I'm not misreading its intent and history...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 19:06 ` Al Viro
@ 2012-02-13 9:40 ` Jiri Slaby
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby @ 2012-02-13 9:40 UTC (permalink / raw)
To: Al Viro
Cc: Jiri Slaby, Richard Weinberger, linux-kernel,
user-mode-linux-devel, akpm, alan, gregkh
On 02/12/2012 08:06 PM, Al Viro wrote:
> Yecchhh... If I'm reading (and grepping) it right, there are only two
> non-default instance of tty_operations ->shutdown() - pty and vt ones.
> Lovely... And while we are at it, vt instance is definitely not safe
> from interrupts - calls console_lock(). Not that it was relevant in
> this case...
Thanks for looking into that. I was too lazy to do that on Sunday.
You're right that it may cause problems. Fortunately vt doesn't refcount
ttys. Hence con_shutdown can be called only from release_tty (close
path) in the user context.
Adding to my TODO list, unless somebody beats me to fix it.
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 0:21 Richard Weinberger
` (2 preceding siblings ...)
2012-02-12 1:02 ` Al Viro
@ 2012-02-12 19:11 ` Al Viro
2012-02-13 9:15 ` Jiri Slaby
3 siblings, 1 reply; 11+ messages in thread
From: Al Viro @ 2012-02-12 19:11 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, user-mode-linux-devel, akpm, alan, gregkh
On Sun, Feb 12, 2012 at 01:21:10AM +0100, Richard Weinberger wrote:
> @@ -343,7 +267,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> {
> struct chan *chan = data;
> struct line *line = chan->line;
> - struct tty_struct *tty = line->tty;
> + struct tty_struct *tty = tty_port_tty_get(&line->port);
> int err;
>
> /*
> @@ -354,6 +278,9 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> spin_lock(&line->lock);
> err = flush_buffer(line);
> if (err == 0) {
> + tty_kref_put(tty);
> +
> + spin_unlock(&line->lock);
> return IRQ_NONE;
> } else if (err < 0) {
> line->head = line->buffer;
> @@ -365,9 +292,12 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
> return IRQ_NONE;
>
> tty_wakeup(tty);
> + tty_kref_put(tty);
> return IRQ_HANDLED;
> }
That, BTW, smells ugly. Note that return before the last one has no
tty_kref_put() for a very good reason - it's under if (!tty). And
just as line->tty, port->tty can become NULL, so tty_port_tty_get()
can, indeed, return NULL here. Which makes the first tty_kref_put()
oopsable...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: your mail
2012-02-12 19:11 ` Al Viro
@ 2012-02-13 9:15 ` Jiri Slaby
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby @ 2012-02-13 9:15 UTC (permalink / raw)
To: Al Viro
Cc: Richard Weinberger, linux-kernel, user-mode-linux-devel, akpm,
alan, gregkh
On 02/12/2012 08:11 PM, Al Viro wrote:
> On Sun, Feb 12, 2012 at 01:21:10AM +0100, Richard Weinberger wrote:
>
>> @@ -343,7 +267,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
>> {
>> struct chan *chan = data;
>> struct line *line = chan->line;
>> - struct tty_struct *tty = line->tty;
>> + struct tty_struct *tty = tty_port_tty_get(&line->port);
>> int err;
>>
>> /*
>> @@ -354,6 +278,9 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
>> spin_lock(&line->lock);
>> err = flush_buffer(line);
>> if (err == 0) {
>> + tty_kref_put(tty);
>> +
>> + spin_unlock(&line->lock);
>> return IRQ_NONE;
>> } else if (err < 0) {
>> line->head = line->buffer;
>> @@ -365,9 +292,12 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
>> return IRQ_NONE;
>>
>> tty_wakeup(tty);
>> + tty_kref_put(tty);
>> return IRQ_HANDLED;
>> }
>
> That, BTW, smells ugly. Note that return before the last one has no
> tty_kref_put() for a very good reason - it's under if (!tty). And
> just as line->tty, port->tty can become NULL, so tty_port_tty_get()
> can, indeed, return NULL here. Which makes the first tty_kref_put()
> oopsable...
Nope, it is allowed to call tty_kref_put(NULL).
regards,
--
js
suse labs
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-02-13 9:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-12 0:21 Richard Weinberger
2012-02-12 0:24 ` [PATCH] um: Use tty_port Richard Weinberger
2012-02-12 13:01 ` Jiri Slaby
2012-02-12 13:12 ` Richard Weinberger
2012-02-12 0:25 ` your mail Jesper Juhl
2012-02-12 1:02 ` Al Viro
2012-02-12 12:40 ` Jiri Slaby
2012-02-12 19:06 ` Al Viro
2012-02-13 9:40 ` Jiri Slaby
2012-02-12 19:11 ` Al Viro
2012-02-13 9:15 ` Jiri Slaby
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).