review of alphatrack.c

[Oliver Neukum <oliver@xxxxxxxxxx>]

/* Use our own dbg macro */
#define dbg_info(dev, format, arg...) do \
    { if (debug) dev_info(dev , format , ## arg); } while (0)

Why?

static void usb_alphatrack_abort_transfers(struct usb_alphatrack *dev)
{
	/* shutdown transfer */
	if (dev->interrupt_in_running) {
		dev->interrupt_in_running = 0;
		if (dev->intf)
			usb_kill_urb(dev->interrupt_in_urb);
	}
	if (dev->interrupt_out_busy)
		if (dev->intf)
			usb_kill_urb(dev->interrupt_out_urb);
}

Don't do this conditionally. There's a small race where you perform
a wakeup on a freed queue.

		if (memcmp
		    (dev->interrupt_in_buffer, dev->oldi_buffer,
		     INPUT_CMD_SIZE) == 0) {
			goto resubmit;
		}
		memcpy(dev->oldi_buffer, dev->interrupt_in_buffer,
		       INPUT_CMD_SIZE);

Can these be combined?

static int usb_alphatrack_open(struct inode *inode, struct file *file)
{
	struct usb_alphatrack *dev;
	int subminor;
	int retval = 0;
	struct usb_interface *interface;

	nonseekable_open(inode, file);
	subminor = iminor(inode);

	mutex_lock(&disconnect_mutex);

usbcore does this for you

	dev = usb_get_intfdata(interface);

	if (!dev) {
		retval = -ENODEV;
		goto unlock_disconnect_exit;
	}

not needed if you deregister the first thing in disconnect

	/* lock this device */
	if (down_interruptible(&dev->sem)) {
		retval = -ERESTARTSYS;
		goto unlock_disconnect_exit;
	}

not much point in making this interruptible

	if (down_interruptible(&dev->sem)) {
		retval = -ERESTARTSYS;
		goto exit;
	}

static int usb_alphatrack_release(struct inode *inode, struct file *file)
{
	struct usb_alphatrack *dev;
	int retval = 0;

	dev = file->private_data;

	if (dev == NULL) {
		retval = -ENODEV;
		goto exit;
	}

	if (down_interruptible(&dev->sem)) {
		retval = -ERESTARTSYS;
		goto exit;
	}

no point in doing this interruptible

	/* wait until write transfer is finished */
	if (dev->interrupt_out_busy)
		wait_event_interruptible_timeout(dev->write_wait,
						 !dev->interrupt_out_busy,
						 2 * HZ);
	usb_alphatrack_abort_transfers(dev);

a harmless signal can wipe the output buffer

/**
 *	usb_alphatrack_poll
 */
static unsigned int usb_alphatrack_poll(struct file *file, poll_table * wait)

should indicate an error condition in case of disconnection

	/* FIXME - there are more usb_alloc routines for dma correctness.
	   Needed? */
	dev->ring_buffer =
	    kmalloc((true_size * sizeof(struct alphatrack_icmd)), GFP_KERNEL);

questionable on cache coherency grounds

	/* if the device is not opened, then we clean up right now */
	if (!dev->open_count) {
		up(&dev->sem);
		usb_alphatrack_delete(dev);
	} else {
		dev->intf = NULL;
		up(&dev->sem);
	}

	atomic_set(&dev->writes_pending, 0);

atomic write to freed memory

You asked for it.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html