Hi Steven,
Em Wed, 9 Aug 2006 14:50:54 +1000
Steven Haigh <[EMAIL PROTECTED]> escreveu:
| Patch is against 2.6.17.7
There's a hunk failing for the Makefile change when applying it in
the latest -rc kernel.
As this will be applied there, I recomend you to rediff.
| + * History:
| + *
| + * 2006-08-07 - 0.0.13 - shaigh
| + * - fixed various errors to compile on kernel 2.6.17.7
| + * 2006-02-19 - 0.0.12 - jhomppi
| + * - remove the mode parameter from the usb_class_driver structure
| + * (for kernel version 2.6.15)
| + * 2006-02-03 - 0.0.11 jhomppi
| + * - remove the serial number retrieval "short-cut"
| + * 2006-01-16 - 0.0.10 jhomppi
| + * - use vendor/product table in adu_probe
| + * - check LINUX_VERSION_CODE for version of linux kernel
| + * and utilize appropriate termination of transfer
| + * 2005-12-02 - 0.0.9 jhomppi
| + * - pre-stage an input read URB to catch first read operation
| + * 2005-11-21 - 0.0.8 jhomppi
| + * - add ADU120,ADU130,ADU208,ADU218 to supported device list
| + * 2005-11-20 - 0.0.7 jhomppi
| + * - remove references to URB_ASYNC_UNLINK
| + * - add MODULE_LICENSE macro to identify our license to kernel
| + * 2005-09-20 - 0.0.6 jhomppi
| + * - replace deprecated usb_unlink_urb with usb_kill_urb
| + * 2003-12-23 - 0.0.5 jhomppi
| + * - urb after each read and write
| + * 2003-12-02 - 0.0.4 jhomppi
| + * - replace urb_submitted with urb status==-EINPROGRESS to stay
| + * in synch with real urb state
| + * - allow multiple open handles to the same device (now that
| + * urb_submitted problem has been removed)
| + * 2003-11-28 - 0.0.3 jhomppi
| + * - applied more changes suggested by Oliver Neukum
| + * - pre-start read if a buffer is available
| + * - correct the location of spin_unlock at end of a read
| + * - added hack to return serial number to caller if read buffer
| + * starts with an 's' (allows serial# retrieval without ioctl)
| + * - check that udev was not nuked by an early disconnect
| + * (ie. check for a null dev->udev in adu_abort_transfers)
| + * - increase scope of lock_irqsave in adu_read so that
| + * urb_submitted is protected versus the interrupt_in_callback
| + * - use down_interruptible where appropriate to prevent locking
| + * driver module into memory forever
| + * 2003-11-27 - 0.0.2 jhomppi
| + * - applied changes suggested by Oliver Neukum to add
| + * spin locks and double buffering to adu_interrupt_in_callback
| + * 2003-11-25 - 0.0.1 jhomppi
| + * - copied from the Lego Tower USB driver for Kernel 2.6.0
| + * - modified to support ADU devices (basically changed only
| + * the names and constants)
| + */
You don't need this, we'll track history in GIT. ;)
| +/* prevent races between open() and disconnect */
| +static DECLARE_MUTEX (disconnect_sem);
I think this can be a mutex intead of a semaphore.
| +/* local function prototypes */
| +static ssize_t adu_read (struct file *file, char *buffer, size_t count,
loff_t *ppos);
| +static ssize_t adu_write (struct file *file, const char *buffer, size_t
count, loff_t *ppos);
| +static int adu_ioctl (struct inode *inode, struct file *file, unsigned int
cmd, unsigned long arg);
| +static inline void adu_delete (struct adu_device *dev);
| +static int adu_open(struct inode *inode, struct file *file);
| +static int adu_release(struct inode *inode, struct file *file);
| +static int adu_release_internal (struct adu_device *dev);
| +static void adu_abort_transfers (struct adu_device *dev);
| +static void adu_interrupt_in_callback (struct urb *urb, struct pt_regs
*regs);
| +static void adu_interrupt_out_callback (struct urb *urb, struct pt_regs
*regs);
| +
| +static int adu_probe (struct usb_interface *interface, const struct
usb_device_id *id);
| +static void adu_disconnect (struct usb_interface *interface);
CodyingStyle hints:
Functions prototypes and definitions are:
my_func()
and not
my_func ()
| +/* file operations needed when we register this driver */
| +static struct file_operations adu_fops = {
| + .owner = THIS_MODULE,
| + .read = adu_read,
| + .write = adu_write,
| + .ioctl = adu_ioctl,
| + .open = adu_open,
| + .release = adu_release,
| +};
make this const please.
| +/**
| + * adu_debug_data
| + */
| +static inline void adu_debug_data (int level, const char *function, int
size, const unsigned char *data)
| +{
| + int i;
| +
| + if (debug < level)
| + return;
| +
| + printk (KERN_DEBUG __FILE__": %s - length = %d, data = ", function,
size);
| + for (i = 0; i < size; ++i) {
| + printk ("%.2x ", data[i]);
| + }
| + printk ("\n");
| +} /* end adu_debug_data */
Are you sure you want this inline?
| +/**
| + * adu_delete
| + */
| +static inline void adu_delete (struct adu_device *dev)
| +{
Ditto.
| + /* free data structures */
| + if (dev->interrupt_in_urb != NULL) {
| + usb_free_urb (dev->interrupt_in_urb);
| + }
| + if (dev->interrupt_out_urb != NULL) {
| + usb_free_urb (dev->interrupt_out_urb);
| + }
usb_free_urb() already does the check for you.
| +static int adu_open (struct inode *inode, struct file *file)
| +{
| + struct adu_device *dev = NULL;
Why the NULL init here?
| + down (&disconnect_sem); /* not interruptible */
No need for the comment.
| + interface = usb_find_interface (&adu_driver, subminor);
| +
| + if (!interface) {
| + err ("%s - error, can't find device for minor %d",
| + __FUNCTION__, subminor);
| + retval = -ENODEV;
| + goto exit_no_device;
| + }
Why the blank line?
| + dev = usb_get_intfdata(interface);
| +
| + if (!dev) {
| + retval = -ENODEV;
| + goto exit_no_device;
| + }
Ditto.
| + int iCount;
| + iCount = atomic_read(&dev->sem.count);
| + dbg(2,"%s : sem down failed, count %d", __FUNCTION__, iCount);
Are you sure you need this?
| +/**
| + * adu_release
| + */
| +static int adu_release (struct inode *inode, struct file *file)
| +{
| + struct adu_device *dev = NULL;
| + int retval = 0;
| +
| + dbg(2," %s : enter", __FUNCTION__);
| +
| + if (file == NULL) {
| + dbg(1," %s : file is NULL", __FUNCTION__);
| + retval = -ENODEV;
| + goto exit;
| + }
'if (!file)' is preferred.
| + if (dev->open_count <= 0) {
| + dbg(1," %s : device not opened", __FUNCTION__);
| + retval = -ENODEV;
| + goto exit;
| + }
This is an error, you should print it with err() IMO.
Also, how could 'dev->open_count' become negative? Maybe you should
WARN_ON() here.
| +/**
| + * adu_abort_transfers
| + * aborts transfers and frees associated data structures
| + */
| +static void adu_abort_transfers (struct adu_device *dev)
| +{
| + dbg(2," %s : enter", __FUNCTION__);
| +
| + if (dev == NULL) {
| + dbg(1," %s : dev is null", __FUNCTION__);
| + goto exit;
| + }
| +
| + if (dev->udev == NULL) {
| + dbg(1," %s : udev is null", __FUNCTION__);
| + goto exit;
| + }
| +
| + dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
| + if (dev->udev->state == USB_STATE_NOTATTACHED) {
| + dbg(1," %s : udev is not attached", __FUNCTION__);
| + goto exit;
| + }
| +
| + /* shutdown transfer */
| +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,10)
| + if (dev->interrupt_in_urb != NULL) {
| + usb_unlink_urb (dev->interrupt_in_urb);
| + }
| + if (dev->interrupt_out_urb != NULL) {
| + usb_unlink_urb (dev->interrupt_out_urb);
| + }
| +#else
| + if (dev->interrupt_in_urb != NULL) {
| + usb_kill_urb (dev->interrupt_in_urb);
| + }
| + if (dev->interrupt_out_urb != NULL) {
| + usb_kill_urb (dev->interrupt_out_urb);
| + }
| +#endif
Hmm. Why the #if/#endif?
| +/**
| + * adu_read
| + */
| +static ssize_t adu_read (struct file *file, char *buffer, size_t count,
loff_t *ppos)
| +{
| + struct adu_device *dev;
| + size_t bytes_read = 0;
| + size_t bytes_to_read = count;
| + int i;
| + int retval = 0;
| + int timeout = 0;
| + int should_submit = 0;
| + unsigned long flags;
| + DECLARE_WAITQUEUE(wait, current);
| +
| + dbg(2," %s : enter, count = %d, file=&%08x", __FUNCTION__, count, file);
| +
| + dev = (struct adu_device *)file->private_data;
| + dbg(2," %s : dev=%08x", __FUNCTION__, dev);
| + /* lock this object */
| + if (down_interruptible(&dev->sem))
| + return -ERESTARTSYS;
| +
| + /* verify that the device wasn't unplugged */
| + if (dev->udev == NULL || dev->minor == 0) {
| + retval = -ENODEV;
| + err("No device or device unplugged %d", retval);
| + goto exit;
| + }
| +
| + /* verify that some data was requested */
| + if (count == 0) {
| + dbg(1," %s : read request of 0 bytes", __FUNCTION__);
| + goto exit;
| + }
| +
| + timeout = COMMAND_TIMEOUT;
| + dbg(2," %s : about to start looping", __FUNCTION__);
| + while (bytes_to_read) {
| + int data_in_secondary = dev->secondary_tail -
dev->secondary_head;
| + dbg(2," %s : while, data_in_secondary=%d, status=%d",
__FUNCTION__, data_in_secondary,
| + dev->interrupt_in_urb->status);
| + if (data_in_secondary) { /* drain secondary buffer */
| + int amount = bytes_to_read < data_in_secondary ?
bytes_to_read : data_in_secondary;
| + i = copy_to_user (buffer,
dev->read_buffer_secondary+dev->secondary_head, amount);
| + dev->secondary_head += (amount - i);
| + bytes_read += (amount - i);
| + bytes_to_read -= (amount - i);
| + if (i) {
| + retval = bytes_read ? bytes_read : -EFAULT;
| + goto exit;
| + }
| + } else { /* we check the primary buffer */
| + spin_lock_irqsave (&dev->buflock, flags);
| + if (dev->read_buffer_length) { /* we secure access to
the primary */
| + char *tmp;
| + dbg(2," %s : swap, read_buffer_length = %d",
__FUNCTION__, dev->read_buffer_length);
| + tmp = dev->read_buffer_secondary;
| + dev->read_buffer_secondary =
dev->read_buffer_primary;
| + dev->read_buffer_primary = tmp;
| + dev->secondary_head = 0;
| + dev->secondary_tail = dev->read_buffer_length;
| + dev->read_buffer_length = 0;
| + spin_unlock_irqrestore (&dev->buflock, flags);
| + should_submit = 1; /* we have a free buffer so
use it */
| + } else { /* even the primary was empty - we may need to
do IO */
| + if (dev->interrupt_in_urb->status ==
-EINPROGRESS) { /*somebody is doing IO*/
| + spin_unlock_irqrestore (&dev->buflock,
flags);
| + dbg(2," %s : submitted already",
__FUNCTION__);
| + } else { /* we must initiate input */
| + dbg(2," %s : initiate input",
__FUNCTION__);
| + dev->read_urb_finished = 0;
| +
| + usb_fill_int_urb
(dev->interrupt_in_urb,dev->udev,
| + usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
| + dev->interrupt_in_buffer,
| +
dev->interrupt_in_endpoint->wMaxPacketSize,
| + adu_interrupt_in_callback,
| + dev,
| +
dev->interrupt_in_endpoint->bInterval);
| + retval =
usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
| + if (!retval) {
| + spin_unlock_irqrestore
(&dev->buflock, flags);
| + dbg(2," %s : submitted OK",
__FUNCTION__);
| + } else {
| + if (retval == -ENOMEM) {
| + retval = bytes_read ?
bytes_read : -ENOMEM;
| + }
| + spin_unlock_irqrestore
(&dev->buflock, flags);
| + dbg(2," %s : submit failed",
__FUNCTION__);
| + goto exit;
| + }
| + }
| +
| + /* we wait for I/O to complete */
| + set_current_state(TASK_INTERRUPTIBLE);
| + add_wait_queue(&dev->read_wait, &wait);
| + if (!dev->read_urb_finished) {
| + timeout =
schedule_timeout(COMMAND_TIMEOUT);
| + }
| + else
| + set_current_state(TASK_RUNNING);
| + remove_wait_queue(&dev->read_wait, &wait);
| +
| + if (timeout <= 0) {
| + dbg(2," %s : timeout", __FUNCTION__);
| + retval = bytes_read ? bytes_read :
-ETIMEDOUT;
| + goto exit;
| + }
| +
| + if (signal_pending(current)) {
| + dbg(2," %s : signal pending",
__FUNCTION__);
| + retval = bytes_read ? bytes_read :
-EINTR;
| + goto exit;
| + }
| + }
| + }
| + }
| +
| + retval = bytes_read;
| + /* if the primary buffer is empty then use it */
| + if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
| + usb_fill_int_urb (dev->interrupt_in_urb,dev->udev,
| + usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
| + dev->interrupt_in_buffer,
| + dev->interrupt_in_endpoint->wMaxPacketSize,
| + adu_interrupt_in_callback,
| + dev,
| + dev->interrupt_in_endpoint->bInterval);
| + /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
| + dev->read_urb_finished = 0;
| + usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
| + /* we ignore failure */
| + }
| +
| + exit:
| + /* unlock the device */
| + up (&dev->sem);
| +
| + dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
| + return retval;
| +} /* end adu_read */
At first, this function looks pretty big, and some 'if' bodies goes too
deep. You might consider breaking it in small pieces and/or review its
logic.
| +static int adu_ioctl (struct inode *inode, struct file *file, unsigned int
cmd, unsigned long arg)
| +{
| + int retval = -ENOTTY; /* default: we don't understand ioctl */
| +
| + return retval;
| +} /* end adu_ioctl */
Return -ENOIOCTLCMD please.
Hmmm can't you omit the ioctl()?
| +/**
| + * adu_probe
| + *
| + * Called by the usb core when a new device is connected that it thinks
| + * this driver might be interested in.
| + */
| +static int adu_probe (struct usb_interface *interface, const struct
usb_device_id *id)
| +{
| + struct usb_device *udev = interface_to_usbdev(interface);
| + struct adu_device *dev = NULL;
| + struct usb_host_interface *iface_desc;
| + struct usb_endpoint_descriptor *endpoint;
| + int i;
| + int iSize;
| + int retval = -ENOMEM;
| +
| + dbg(2," %s : enter", __FUNCTION__);
| +
| + if (udev == NULL) {
| + info ("udev is NULL.");
| + retval = -ENODEV;
| + goto exit;
| + }
| +
| + iSize = sizeof(device_table) / sizeof(device_table[0]);
| + for (i = 0; i < iSize; i++) {
| + if ((udev->descriptor.idVendor == device_table[i].idVendor) &&
| + (udev->descriptor.idProduct == device_table[i].idProduct))
| + break;
| + }
| + if (i >= iSize) return -ENODEV;
| +
| + /* allocate memory for our device state and intialize it */
| +
| + dev = kmalloc (sizeof(struct adu_device), GFP_KERNEL);
| +
| + if (dev == NULL) {
| + err ("Out of memory");
| + goto exit;
| + }
| +
| + init_MUTEX (&dev->sem);
| + spin_lock_init (&dev->buflock);
| +
| + dev->udev = udev;
| + dev->open_count = 0;
| +
| + dev->read_buffer_primary = NULL;
| + dev->read_buffer_secondary = NULL;
| + dev->read_buffer_length = 0;
| + dev->secondary_head = 0;
| + dev->secondary_tail = 0;
| +
| + init_waitqueue_head (&dev->read_wait);
| + init_waitqueue_head (&dev->write_wait);
| +
| + dev->interrupt_in_buffer = NULL;
| + dev->interrupt_in_endpoint = NULL;
| + dev->interrupt_in_urb = NULL;
| + dev->read_urb_finished = 0;
| +
| + dev->interrupt_out_buffer = NULL;
| + dev->interrupt_out_endpoint = NULL;
| + dev->interrupt_out_urb = NULL;
If you use kzalloc() here, you can drop most of this init code.
| + // JJH - debug code prime the buffer
| + memset(dev->read_buffer_primary, 'a',
dev->interrupt_in_endpoint->wMaxPacketSize);
| +
memset(dev->read_buffer_primary+dev->interrupt_in_endpoint->wMaxPacketSize, 'b',
| + dev->interrupt_in_endpoint->wMaxPacketSize);
| +
memset(dev->read_buffer_primary+2*dev->interrupt_in_endpoint->wMaxPacketSize,
'c',
| + dev->interrupt_in_endpoint->wMaxPacketSize);
| +
memset(dev->read_buffer_primary+3*dev->interrupt_in_endpoint->wMaxPacketSize,
'd',
| + dev->interrupt_in_endpoint->wMaxPacketSize);
| +
| + dev->read_buffer_secondary = kmalloc
((4*dev->interrupt_in_endpoint->wMaxPacketSize), GFP_KERNEL);
| + if (!dev->read_buffer_secondary) {
| + err("Couldn't allocate read_buffer_secondary");
| + goto error;
| + }
We do this already, don't we?
--
Luiz Fernando N. Capitulino
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
