/* This version of the Linux driver source contains a number of abominable conditional compilation sections to manage the API changes between kernel versions 2.6.18, 2.6.25, and the latest (currently 2.6.27). At some point we'll hand a version of this driver off to the mainline Linux source tree, and we'll strip all these sections out. For now it makes it much easier to keep it all in sync while the driver is being developed. */ It doesn't seem to #include <linux/usb/cdc.h> #ifndef CDC_DATA_INTERFACE_TYPE #define CDC_DATA_INTERFACE_TYPE 0x0a #endif I was wondering why this wasn't using CDC-ACM ? struct vizzini_serial_private { struct usb_interface *data_interface; }; Perhaps that can get simplified ? static struct vizzini_baud_rate vizzini_baud_rates[] = { Some explanation might be useful ? static int vizzini_set_baud_rate(struct usb_serial_port *port, And the actual rate hould get passed back in the termios (tty_termios_encode*) static void vizzini_set_termios(struct tty_struct *tty_param, struct usb_serial_port *port, struct ktermios *old_termios) { struct tty_struct *tty = port->port.tty; This mistake is all over the driver. port->port.tty is unsafe as it may change under you. That's *why* we pass a safe tty reference that the driver then ignores. If the driver disable/enable can lose bytes it also needs to figure out if a real hardware change has occurred or some apps will get upset. } else if ((cflag & CSIZE) == CS5) { /* Enabling 5-bit mode is really 9-bit mode! */ format_size = UART_FORMAT_SIZE_9; If this is magic hackery for 9bit char then it needs doing properly, if it's a clever way to get 5bit then fine. } else { format_size = UART_FORMAT_SIZE_8; } And for unsupported formats we need to report the format we actually picked in the tty->termios data. static int vizzini_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) { struct usb_serial_port *port = tty->driver_data; struct vizzini_port_private *portdata = usb_get_serial_port_data(port); struct serial_struct ss; dev_dbg(&port->dev, "%s %08x\n", __func__, cmd); switch (cmd) { case TIOCGSERIAL: if (!arg) return -EFAULT; memset(&ss, 0, sizeof(ss)); ss.baud_base = portdata->baud_base; This leaves lots of fields blank rather than correctly filled in as the apps will expect. If we support it then it needs to support it all for read even if only writing odd bits. if (copy_to_user((void __user *)arg, &ss, sizeof(ss))) return -EFAULT; break; case TIOCSSERIAL: if (!arg) return -EFAULT; if (copy_from_user(&ss, (void __user *)arg, sizeof(ss))) return -EFAULT; portdata->baud_base = ss.baud_base; Because setting stuff as non superuser without any validation is good. Should also support low latency config if we are doing this. #ifdef VIZZINI_IWA static const int vizzini_parity[] = { 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0 }; #endif Doing parity is something I never bothered with but if we do it then we ought to have a helper in the tty core instead. Also it's stupid to use ints for this as it just blows more cache. In fact you might as well use bits and just test_bit() for parity. static int vizzini_write_room(struct tty_struct *tty) { This seems dodgy - it must never report more than it can guarantee will fit. If it ever reports 2048 it can't un-report them as free until those 2048 bytes are written. Probably it should be using the kfifo code ? buffer = kmalloc(bufsize, GFP_ATOMIC); A fifo would also avoid most of these problems as you can just retry tidily. This whole path is really ugly to be honest as it gets called with irqs off by code expecting the queueing to be fast. static void vizzini_in_callback(struct urb *urb) { int endpoint = usb_pipeendpoint(urb->pipe); struct usb_serial_port *port = urb->context; struct vizzini_port_private *portdata = usb_get_serial_port_data(port); struct tty_struct *tty = port->port.tty; No kref taken so unsafe, could also be NULL, so you execute NULL->functiontptr. Not good on platforms that don't have low memory mapping blocked ! room = tty_buffer_request_room(tty, length); if (room != length) dev_dbg(&port->dev, "Not enough room in TTY buf, dropped %d chars.\n", length - room); if (room) { Not worth checking, just shovel it in - the buffer code will do the work and only fail when its really congested and the box is stuffed up. Plus your dev_dbg here will deadlock if this is a USB serial console I think ? static void vizzini_int_callback(struct urb *urb) { struct usb_serial_port *port = urb->context; struct vizzini_port_private *portdata = usb_get_serial_port_data(port); struct tty_struct *tty = port->port.tty; Again kref... : dev_dbg(&port->dev, "Resubmitting interrupt IN urb %p\n", urb); status = usb_submit_urb(urb, GFP_ATOMIC); if (status) dev_err(&port->dev, "usb_submit_urb failed with result %d", status); Whats the error recovery path for this - a single fail kills the port ? } static int vizzini_open(struct tty_struct *tty_param, struct usb_serial_port *port) { struct vizzini_port_private *portdata; struct usb_serial *serial = port->serial; struct tty_struct *tty = port->port.tty; Again we pass the tty for a reason ! static void vizzini_close(struct usb_serial_port *port) { int i; struct usb_serial *serial = port->serial; struct vizzini_port_private *portdata; struct tty_struct *tty = port->port.tty; You can't safelty reference tty from here, but it's not used anyway so just kill it off. portdata = usb_get_serial_port_data(port); acm_set_control(port, portdata->ctrlout = 0); if (serial->dev) { /* Stop reading/writing urbs */ for (i = 0; i < N_IN_URB; i++) usb_kill_urb(portdata->in_urbs[i]); } usb_kill_urb(port->interrupt_in_urb); tty = NULL; /* FIXME */ Alan -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html