Messages in this thread | | | Date | Fri, 30 Jun 2006 00:10:21 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers |
| |
On Fri, 30 Jun 2006 01:48:02 -0400 Andy Gay <andy@andynet.net> wrote:
> > Adapted from an earlier patch by Greg KH <gregkh@suse.de>. > That patch added multiple read urbs and larger transfer buffers to allow > data transfers at full EvDO speed. > > This version includes additional device IDs and fixes a memory leak in > the transfer buffer allocation. > > Some (maybe all?) of the supported devices present multiple bulk endpoints, > the additional EPs can be used for control and status functions. > This version allocates 3 EPs by default, that can be changed using > the 'endpoints' module parameter. > > Tested with Sierra Wireless EM5625 and MC5720 embedded modules. > > Device ID (0x0c88, 0x17da) for the Kyocera Wireless KPC650/Passport > was added but is not yet tested. > > ... > > +static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs) > +{ > + struct usb_serial_port *port = urb->context; > + unsigned char *data = urb->transfer_buffer; > + struct tty_struct *tty; > + int result; > + > + dbg("%s - port %d", __FUNCTION__, port->number); > + > + if (urb->status) { > + dbg("%s - nonzero read bulk status received: %d", > + __FUNCTION__, urb->status); > + /* something happened, so free up the memory for this urb /* > + if (urb->transfer_buffer) { > + kfree (urb->transfer_buffer); > + urb->transfer_buffer = NULL; > + } > + return; > + } > + usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data); > + > + tty = port->tty; > + if (tty && urb->actual_length) { > + tty_buffer_request_room(tty, urb->actual_length); > + tty_insert_flip_string(tty, data, urb->actual_length);
Is it correct to ignore the return value from those two functions?
> + tty_flip_buffer_push(tty); > + } > + /* should this use GFP_KERNEL? */ > + result = usb_submit_urb(urb, GFP_ATOMIC);
If possible, yep.
> ... > > +static int airprime_open(struct usb_serial_port *port, struct file *filp) > +{ > + struct airprime_private *priv = usb_get_serial_port_data(port); > + struct usb_serial *serial = port->serial; > + struct urb *urb; > + char *buffer; > + int i; > + int result = 0; > + > + dbg("%s - port %d", __FUNCTION__, port->number); > + > + /* initialize our private data structure if it isn't already created */ > + if (!priv) { > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + spin_lock_init(&priv->lock); > + usb_set_serial_port_data(port, priv); > + } > + /* TODO handle error conditions better, right now we leak memory */ > + for (i = 0; i < NUM_READ_URBS; ++i) { > + buffer = kmalloc(buffer_size, GFP_KERNEL); > + if (!buffer) { > + dev_err(&port->dev, "%s - out of memory.\n", > + __FUNCTION__); > + return -ENOMEM; > + } > + urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!urb) { > + dev_err(&port->dev, "%s - no more urbs?\n", > + __FUNCTION__); > + return -ENOMEM; > + } > + usb_fill_bulk_urb(urb, serial->dev, > + usb_rcvbulkpipe(serial->dev, > + port->bulk_out_endpointAddress), > + buffer, buffer_size, > + airprime_read_bulk_callback, port); > + result = usb_submit_urb(urb, GFP_KERNEL); > + if (result) { > + dev_err(&port->dev, > + "%s - failed submitting read urb %d for port %d, error %d\n", > + __FUNCTION__, i, port->number, result); > + return result; > + } > + /* fun with reference counting, when this urb is finished, the > + * host driver will free it up automatically */ > + /* don't do this here, we need the urb to stay around until the close > + function can take care of it */ > + //usb_free_urb (urb); > + /* instead remember this urb so we can kill it when the > + port is closed */ > + priv->read_urbp[i] = urb; > + } > + return result; > +} > +
This function leaks memory all over the place if something goes wrong.
Please redesign it to have a single `return' statement. You'll find that'll help avoid leaks now and during any later enhancements.
> +{ > + struct airprime_private *priv = usb_get_serial_port_data(port); > + int i; > + > + dbg("%s - port %d", __FUNCTION__, port->number); > + > + /* killing the urb will invoke read_bulk_callback() with an error status, > + so the transfer buffer will be freed there */ > + for (i = 0; i < NUM_READ_URBS; ++i) { > + usb_kill_urb (priv->read_urbp[i]); > + usb_free_urb (priv->read_urbp[i]); > + } > + > + /* free up private structure? */
Yes please ;)
> +} > + > +static int airprime_write(struct usb_serial_port *port, > + const unsigned char *buf, int count) > +{ > + struct airprime_private *priv = usb_get_serial_port_data(port); > + struct usb_serial *serial = port->serial; > + struct urb *urb; > + unsigned char *buffer; > + unsigned long flags; > + int status; > + dbg("%s - port %d", __FUNCTION__, port->number); > + > + spin_lock_irqsave(&priv->lock, flags); > + if (priv->outstanding_urbs > NUM_WRITE_URBS) { > + spin_unlock_irqrestore(&priv->lock, flags); > + dbg("%s - write limit hit\n", __FUNCTION__); > + return 0; > + } > + spin_unlock_irqrestore(&priv->lock, flags); > + buffer = kmalloc(count, GFP_ATOMIC); > + if (!buffer) { > + dev_err(&port->dev, "out of memory\n"); > + return -ENOMEM; > + } > + urb = usb_alloc_urb(0, GFP_ATOMIC); > + if (!urb) { > + dev_err(&port->dev, "no more free urbs\n"); > + kfree (buffer); > + return -ENOMEM; > + } > + memcpy (buffer, buf, count); > + > + usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, buffer); > + > + usb_fill_bulk_urb(urb, serial->dev, > + usb_sndbulkpipe(serial->dev, > + port->bulk_out_endpointAddress), > + buffer, count, > + airprime_write_bulk_callback, port); > + > + /* send it down the pipe */ > + status = usb_submit_urb(urb, GFP_ATOMIC); > + if (status) { > + dev_err(&port->dev, > + "%s - usb_submit_urb(write bulk) failed with status = %d\n", > + __FUNCTION__, status); > + count = status; > + kfree (buffer); > + } else { > + spin_lock_irqsave(&priv->lock, flags); > + ++priv->outstanding_urbs; > + spin_unlock_irqrestore(&priv->lock, flags); > + } > + /* we are done with this urb, so let the host driver > + * really free it when it is finished with it */ > + usb_free_urb (urb); > + return count; > +}
Is usb_serial_driver.write() really called in a context in which it is forced to use GFP_ATOMIC?
Again, implementing this function as single-exit would make it easier to maintain.
> +MODULE_PARM_DESC(debug, "Debug enabled or not");
Just "Debug enabled".
> +module_param(buffer_size, int, 0); > +MODULE_PARM_DESC(buffer_size, "Size of the transfer buffers");
Units?
- 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/
| |