Dear Oliver:
Thank you for the code review.
I will dust off the alphatrack and get running a new kernel and hack
some this week.
It looks like most of your suggestions also apply to
tranzport.c. I will tackle both. Further comments below.
Oliver Neukum <oliver@xxxxxxxxxx> writes:
> /* Use our own dbg macro */
> #define dbg_info(dev, format, arg...) do \
> { if (debug) dev_info(dev , format , ## arg); } while (0)
>
> Why?
This lived out of tree for a very long time. Still does.
Will nuke.
>
> 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.
Can you suggest a usb driver to look at that is doing this right? (this
same question applies to most other comments in your review). I had
based this off of code that were the current examples of exemplary
behavior back in ancient days.
>
> 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?
Probably.I will look. At one point both of these drivers did a great
deal more with compressing event history and did it at interrupt time
rather than later, and I think this is an artifact or requirement of
that. Most likely an artifact.
>
> 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
When did it start doing that?
> dev = usb_get_intfdata(interface);
>
> if (!dev) {
> retval = -ENODEV;
> goto unlock_disconnect_exit;
> }
>
> not needed if you deregister the first thing in disconnect
OK.
>
> /* lock this device */
> if (down_interruptible(&dev->sem)) {
> retval = -ERESTARTSYS;
> goto unlock_disconnect_exit;
> }
>
> not much point in making this interruptible
Artifact. I'll have to wrap my head around it again.
>
> 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
good point. That might explain some odd behavior I've seen on shutdown, too.
>
> /**
> * 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
When I wrote this I assumed that the only users would be x86 and x86_64,
and maybe ppc. I guess now people are pushing x86 past 4GB, and this is
also a problem here?
Again, helps to see a driver that is doing it right. Suggestion?
>
> /* 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
Ugh. OK, will fix.
>
> You asked for it.
And I am delighted to have your feedback. I would love to have these
drivers fully baked, done, and in regular distros so the people using
them wouldn't have to leap through hoops anymore. Thank you.
I will make some modifications in the next week.
--
Dave Taht
Postcards from the Bleeding Edge (http://the-edge.blogspot.com)
--
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
