Messages in this thread | | | Date | Sun, 14 Jan 2007 00:37:52 +0000 | From | Alan <> | Subject | Re: [patch 16/20] XEN-paravirt: Add the Xen virtual console driver. |
| |
> +#endif > + tty_insert_flip_char(xencons_tty, buf[i], 0);
Please use the defines like TTY_NORMAL not just 0.
> + if ((xencons_tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) && > + (xencons_tty->ldisc.write_wakeup != NULL)) > + (xencons_tty->ldisc.write_wakeup)(xencons_tty); > + }
You are't allowed to derefence and call ldisc methods without holding the lock. You can replace that chunk with a call to the helper function tty_wakeup(tty). Small but real race condition otherwise as you xencons_tty->ldisc may be changing as you call it.
> +static inline int __xencons_put_char(int ch) > +{ > + char _ch = (char)ch; > + if ((wp - wc) == wbuf_size) > + return 0; > + wbuf[WBUF_MASK(wp++)] = _ch; > + return 1; > +}
A lot of very confusing sign stuff here - you turn an int into a char and put it into a uchar array
> + for (i = 0; i < count; i++) > + if (!__xencons_put_char(buf[i])) > + break;
The int coming from a uchar array
Don't think its wrong - just acutely weird and perhaps could be tidier
> +static void xencons_close(struct tty_struct *tty, struct file *filp) > +{ > + unsigned long flags; > + > + mutex_lock(&tty_mutex);
It would be good in future if you could avoid using tty_mutex and use a private lock. At the moment vt "borrows" it and there are a couple of incestuous spots but the plan is to eventually fix them and make it private to tty_io.
Andrew: No objection to this tty stuff being merged provided the bugs noted above (not worried about the sign stuff) are fixed before it goes on to Linus.
Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |