Messages in this thread | | | Date | Sun, 12 Feb 2012 14:01:09 +0100 | From | Jiri Slaby <> | Subject | Re: [PATCH] um: Use tty_port |
| |
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
| |