2007-10-14 18:03:25

by Vitaliy Ivanov

[permalink] [raw]
Subject: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Hello Willy, Greg and list,

I have ported adutux driver for ADU series device list from 2.6 to 2.4.
More on devices:
http://www.ontrak.net/products.htm#Table%205

Once I needed to make ADU200 work under 2.4 enterprise kernel and wasn't able to do this.
My organization decided to use another device for our private purposes.

I was always interested in kernel hacking and thought it's a good point to start it from.
Just to add I'm not related to Ontrak in anyway. Also I'm not a Google person.
Did it just for fun.

A few technical notes:
- I was trying to leave as much as possible adutux driver code from 2.6 as it's in the 2.6 mainline for a long time and seems like it works OK there.
- Used one shot interrupt urbs that's why all intervals are 0.
- Reused 67 minor number from 2.6 kernel for this device.
- All 2.4 related changes are taken/reused from usb-skeleton.

Patch is based on the latest 2.4.35.3.

Performed a lot of tests but only under UHCI controller and ADU200 and all seems to be OK.

I would like someone to perform code review as it's my first attempt to the kernel programming.
Any comments, propositions are welcome.

Signed-off-by: Vitaliy Ivanov <[email protected]>
Tested-by: Vitaliy Ivanov <[email protected]>

--

diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/Documentation/Configure.help linux-2.4.35.3.build/Documentation/Configure.help
-- KERNELS/linux-2.4.35.3/Documentation/Configure.help 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/Documentation/Configure.help 2007-10-14 19:19:06.000000000 +0300
@@ -16123,6 +16123,17 @@ CONFIG_USB_RIO500
The module will be called rio500.o. If you want to compile it as
a module, say M here and read <file:Documenatation/modules.txt>.

+Ontrak Control Systems ADU devices support
+CONFIG_USB_ADUTUX
+ Say Y here if you want to connect ADU 100/200 series devices to your
+ computer's USB port.
+ Details at: <http://www.ontrak.net/products.htm#Table%205>
+
+ This code is also available as a module ( = code which can be
+ inserted in and removed from the running kernel whenever you want).
+ The module will be called adutux.o. If you want to compile it as
+ a module, say M here and read <file:Documenatation/modules.txt>.
+
Auerswald device support
CONFIG_USB_AUERSWALD
Say Y here if you want to connect an Auerswald USB ISDN Device
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/adutux.c linux-2.4.35.3.build/drivers/usb/adutux.c
-- KERNELS/linux-2.4.35.3/drivers/usb/adutux.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/adutux.c 2007-10-14 19:08:59.000000000 +0300
@@ -0,0 +1,1035 @@
+/*
+ * This is a 2.4.* kernel version of adutux driver that
+ * was originaly created by John Homppi for 2.6.* kernels.
+ *
+ * Copyright (c) 2007 Vitaliy Ivanov ([email protected])
+ *
+ * Original note:
+ *
+ * adutux - driver for ADU devices from Ontrak Control Systems
+ * This is an experimental driver. Use at your own risk.
+ * This driver is not supported by Ontrak Control Systems.
+ *
+ * Copyright (c) 2003 John Homppi (SCO, leave this notice here)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * derived from the Lego USB Tower driver 0.56:
+ * Copyright (c) 2003 David Glance <[email protected]>
+ * 2001 Juergen Stuber <[email protected]>
+ * that was derived from USB Skeleton driver - 0.5
+ * Copyright (c) 2001 Greg Kroah-Hartman ([email protected])
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/devfs_fs_kernel.h>
+#include <linux/usb.h>
+#include <asm/uaccess.h>
+
+#ifdef CONFIG_USB_DEBUG
+static int debug = 5;
+#else
+static int debug = 1;
+#endif
+
+/* Use our own dbg macro */
+#undef dbg
+#define dbg(lvl, format, arg...) \
+do { \
+ if (debug >= lvl) \
+ printk(KERN_DEBUG __FILE__ " : " format " \n", ## arg); \
+} while (0)
+
+
+/* Version Information */
+#define DRIVER_VERSION "v0.0.1"
+#define DRIVER_AUTHOR "Vitaliy Ivanov"
+#define DRIVER_DESC "adutux (see http://www.ontrak.net)"
+
+/* Module parameters */
+module_param(debug, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Debug enabled or not");
+
+/* Define these values to match your device */
+#define ADU_VENDOR_ID 0x0a07
+#define ADU_PRODUCT_ID 0x0064
+
+/* table of devices that work with this driver */
+static struct usb_device_id device_table [] = {
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID) }, /* ADU100 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+20) }, /* ADU120 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+30) }, /* ADU130 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+100) }, /* ADU200 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+108) }, /* ADU208 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+118) }, /* ADU218 */
+ { }/* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+#define ADU_MINOR_BASE 67
+
+/* we can have up to this number of device plugged in at once */
+#define MAX_DEVICES 16
+
+#define COMMAND_TIMEOUT (2*HZ) /* 60 second timeout for a command */
+
+/* Structure to hold all of our device specific stuff */
+struct adu_device {
+ struct semaphore sem; /* locks this structure */
+ struct usb_device* udev; /* save off the usb device pointer */
+ //struct usb_interface* interface;
+ devfs_handle_t devfs; /* devfs device node */
+ unsigned char minor; /* the starting minor number for this device */
+
+ int open_count; /* number of times this port has been opened */
+
+ char* read_buffer_primary;
+ int read_buffer_length;
+ char* read_buffer_secondary;
+ int secondary_head;
+ int secondary_tail;
+ spinlock_t buflock;
+
+ wait_queue_head_t read_wait;
+ wait_queue_head_t write_wait;
+
+ char* interrupt_in_buffer;
+ struct usb_endpoint_descriptor* interrupt_in_endpoint;
+ struct urb* interrupt_in_urb;
+ int read_urb_finished;
+
+ char* interrupt_out_buffer;
+ struct usb_endpoint_descriptor* interrupt_out_endpoint;
+ struct urb* interrupt_out_urb;
+};
+
+/* the global usb devfs handle */
+extern devfs_handle_t usb_devfs_handle;
+
+/* 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 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_interrupt_in_callback(struct urb *urb);
+static void adu_interrupt_out_callback(struct urb *urb);
+static void *adu_probe(struct usb_device *dev, unsigned int ifnum, const struct usb_device_id *id);
+static void adu_disconnect(struct usb_device *dev, void *ptr);
+
+/* array of pointers to our devices that are currently connected */
+static struct adu_device *minor_table[MAX_DEVICES];
+
+/* lock to protect the minor_table structure */
+static DECLARE_MUTEX(minor_table_mutex);
+
+/* file operations needed when we register this driver */
+static struct file_operations adu_fops = {
+ owner: THIS_MODULE,
+ read: adu_read,
+ write: adu_write,
+ open: adu_open,
+ release: adu_release,
+};
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver adu_driver = {
+ name: "adutux",
+ probe: adu_probe,
+ disconnect: adu_disconnect,
+ fops: &adu_fops,
+ minor: ADU_MINOR_BASE,
+ id_table: device_table,
+};
+
+/**
+ * adu_debug_data
+ */
+static 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");
+}
+
+/**
+ * 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;
+ }
+
+ /* shutdown transfer */
+ usb_unlink_urb(dev->interrupt_in_urb);
+ usb_unlink_urb(dev->interrupt_out_urb);
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_delete
+ */
+static void adu_delete(struct adu_device *dev)
+{
+ dbg(2, "%s enter", __FUNCTION__);
+
+ minor_table[dev->minor] = NULL;
+ adu_abort_transfers(dev);
+
+ /* 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);
+ }
+ if (dev->read_buffer_primary != NULL) {
+ kfree(dev->read_buffer_primary);
+ }
+ if (dev->read_buffer_secondary != NULL) {
+ kfree(dev->read_buffer_secondary);
+ }
+ if (dev->interrupt_in_buffer != NULL) {
+ kfree(dev->interrupt_in_buffer);
+ }
+ if (dev->interrupt_out_buffer != NULL) {
+ kfree(dev->interrupt_out_buffer);
+ }
+ kfree(dev);
+
+ dbg(2, "%s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_interrupt_in_callback
+ */
+static void adu_interrupt_in_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+
+ spin_lock(&dev->buflock);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) && (urb->status != -ECONNRESET) &&
+ (urb->status != -ESHUTDOWN)) {
+ dbg(1," %s : nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ if (urb->actual_length > 0 && dev->interrupt_in_buffer[0] != 0x00) {
+ if (dev->read_buffer_length <
+ (4 * le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize)) -
+ (urb->actual_length)) {
+ memcpy (dev->read_buffer_primary +
+ dev->read_buffer_length,
+ dev->interrupt_in_buffer, urb->actual_length);
+
+ dev->read_buffer_length += urb->actual_length;
+ dbg(2," %s reading %d ", __FUNCTION__,
+ urb->actual_length);
+ } else {
+ dbg(1," %s : read_buffer overflow", __FUNCTION__);
+ }
+ }
+
+exit:
+ dev->read_urb_finished = 1;
+ spin_unlock(&dev->buflock);
+ /* always wake up so we recover from errors */
+ wake_up_interruptible(&dev->read_wait);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_interrupt_out_callback
+ */
+static void adu_interrupt_out_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5,__FUNCTION__, urb->actual_length, urb->transfer_buffer);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) &&
+ (urb->status != -ECONNRESET)) {
+ dbg(1, " %s :nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ wake_up_interruptible(&dev->write_wait);
+exit:
+
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_open
+ */
+static int adu_open(struct inode *inode, struct file *file)
+{
+ struct adu_device *dev = NULL;
+ int subminor;
+ int retval = 0;
+
+ dbg(2,"%s : enter", __FUNCTION__);
+
+ subminor = MINOR (inode->i_rdev) - ADU_MINOR_BASE;
+ if ((subminor < 0) ||
+ (subminor >= MAX_DEVICES)) {
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our minor table and get our local data for this minor */
+ down (&minor_table_mutex);
+ dev = minor_table[subminor];
+ if (dev == NULL) {
+ retval = -ENODEV;
+ goto unlock_minor_table_exit;
+ }
+
+ /* lock this device */
+ down (&dev->sem);
+
+ /* increment our usage count for the device */
+ ++dev->open_count;
+ dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
+
+ /* check that nobody else is using the device */
+ /* NOTE: the cleanup code will decrement the count to 1 */
+ if (dev->open_count > 1) {
+ retval = -EBUSY;
+ goto error;
+ }
+
+ /* save device in the file's private structure */
+ file->private_data = dev;
+
+ /* initialize in direction */
+ dev->read_buffer_length = 0;
+
+ /* fixup first read by having urb waiting for it */
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev, dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ if (retval != 0) {
+ err("Couldn't submit interrupt_in_urb");
+ goto error;
+ }
+ /* end of fixup for first read */
+
+ goto unlock_exit;
+
+error:
+ adu_release_internal(dev);
+
+unlock_exit:
+ /* unlock this device */
+ up(&dev->sem);
+
+unlock_minor_table_exit:
+ /* unlock the minor table */
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2, " %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_release_internal
+ */
+static int adu_release_internal(struct adu_device *dev)
+{
+ int retval = 0;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ if (dev->udev == NULL) {
+ /* the device was unplugged before the file was released */
+ adu_delete(dev);
+ goto exit;
+ }
+
+ /* decrement our usage count for the device */
+ --dev->open_count;
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (dev->open_count <= 0) {
+ adu_abort_transfers(dev);
+ dev->open_count = 0;
+ }
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+ return retval;
+}
+
+/**
+ * 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;
+ }
+
+ dev = file->private_data;
+
+ if (dev == NULL) {
+ dbg(1," %s : object is NULL", __FUNCTION__);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our minor table */
+ down(&minor_table_mutex);
+
+ /* lock our device */
+ down(&dev->sem);
+
+ if (dev->open_count <= 0) {
+ dbg(1," %s : device not opened", __FUNCTION__);
+ up(&dev->sem);
+ up(&minor_table_mutex);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* do the work */
+ retval = adu_release_internal(dev);
+
+ /* unlocking device */
+ up(&dev->sem);
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_read
+ */
+static ssize_t adu_read(struct file *file, __user 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 = %Zd, file=%p", __FUNCTION__, count, file);
+
+ dev = file->private_data;
+ dbg(2," %s : dev=%p", __FUNCTION__, dev);
+ /* lock this object */
+ if (down_interruptible(&dev->sem))
+ return -ERESTARTSYS;
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ 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);
+ if (i < 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+ 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);
+ /* we have a free buffer so use it */
+ should_submit = 1;
+ } 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;
+
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev, dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ 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) {
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev,dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ usb_submit_urb(dev->interrupt_in_urb);
+ /* we ignore failure */
+ }
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_write
+ */
+static ssize_t adu_write(struct file *file, const __user char *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct adu_device *dev;
+ size_t bytes_written = 0;
+ size_t bytes_to_write;
+ size_t buffer_size;
+ int retval = 0;
+ int timeout = 0;
+
+ dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
+
+ dev = file->private_data;
+
+ /* lock this object */
+ down_interruptible(&dev->sem);
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ retval = -ENODEV;
+ err("No device or device unplugged %d", retval);
+ goto exit;
+ }
+
+ /* verify that we actually have some data to write */
+ if (count == 0) {
+ dbg(1," %s : write request of 0 bytes", __FUNCTION__);
+ goto exit;
+ }
+
+
+ while (count > 0) {
+ if (dev->interrupt_out_urb->status == -EINPROGRESS) {
+ timeout = COMMAND_TIMEOUT;
+
+ while (timeout > 0) {
+ if (signal_pending(current)) {
+ dbg(1," %s : interrupted", __FUNCTION__);
+ retval = -EINTR;
+ goto exit;
+ }
+ up(&dev->sem);
+ timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ down_interruptible(&dev->sem);
+ if (timeout > 0) {
+ break;
+ }
+ dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
+ }
+
+
+ dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
+
+ if (timeout == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit;
+ }
+
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+
+ } else {
+ dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
+
+ /* write the data into interrupt_out_buffer from userspace */
+ buffer_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
+ bytes_to_write = count > buffer_size ? buffer_size : count;
+ dbg(4," %s : buffer_size = %Zd, count = %Zd, bytes_to_write = %Zd",
+ __FUNCTION__, buffer_size, count, bytes_to_write);
+
+ if (copy_from_user(dev->interrupt_out_buffer, buffer, bytes_to_write) != 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ /* send off the urb */
+ FILL_INT_URB(
+ dev->interrupt_out_urb,
+ dev->udev,
+ usb_sndintpipe(dev->udev, dev->interrupt_out_endpoint->bEndpointAddress),
+ dev->interrupt_out_buffer,
+ bytes_to_write,
+ adu_interrupt_out_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_urb->actual_length = bytes_to_write;
+ retval = usb_submit_urb(dev->interrupt_out_urb);
+ if (retval < 0) {
+ err("Couldn't submit interrupt_out_urb %d", retval);
+ goto exit;
+ }
+
+ buffer += bytes_to_write;
+ count -= bytes_to_write;
+
+ bytes_written += bytes_to_write;
+ }
+ }
+
+ retval = bytes_written;
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_probe
+ *
+ * Called by the usb core when a new device is connected that it thinks
+ * this driver might be interested in.
+ */
+static void *adu_probe(struct usb_device *udev,
+ unsigned int ifnum, const struct usb_device_id *id)
+{
+ struct adu_device *dev = NULL;
+ struct usb_interface *interface;
+ struct usb_interface_descriptor *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ int minor;
+ int in_end_size;
+ int out_end_size;
+ int i;
+ char name[10];
+ void *retval = NULL;
+
+ dbg(2, " %s : enter", __FUNCTION__);
+
+ if (udev == NULL) {
+ info ("udev is NULL.");
+ goto exit;
+ }
+
+ if (ifnum != 0) {
+ info ("Strange interface number %d.", ifnum);
+ goto exit;
+ }
+
+ /* select a "subminor" number (part of a minor number) */
+ down (&minor_table_mutex);
+ for (minor = 0; minor < MAX_DEVICES; ++minor) {
+ if (minor_table[minor] == NULL)
+ break;
+ }
+ if (minor >= MAX_DEVICES) {
+ info ("Too many devices plugged in, can not handle this device.");
+ goto unlock_minor_exit;
+ }
+
+ /* 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 unlock_minor_exit;
+ }
+ memset (dev, 0x00, sizeof (*dev));
+
+ init_MUTEX(&dev->sem);
+ spin_lock_init(&dev->buflock);
+ down(&dev->sem);
+ dev->udev = udev;
+
+ interface = &dev->udev->actconfig->interface[0];
+ iface_desc = &interface->altsetting[0];
+
+ //dev->interface = interface;
+ dev->minor = minor;
+ 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->interrupt_out_buffer = NULL;
+ dev->interrupt_out_endpoint = NULL;
+ dev->interrupt_out_urb = NULL;
+
+ dev->read_urb_finished = 0;
+
+ /* set up the endpoint information */
+ for (i = 0; i < iface_desc->bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i];
+
+ /**
+ * Check if the endpoint has IN direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_IN) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt in endpoint */
+ dev->interrupt_in_endpoint = endpoint;
+ }
+
+ /**
+ * Check if the endpoint has OUT direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt out endpoint */
+ dev->interrupt_out_endpoint = endpoint;
+ }
+ }
+
+ if(dev->interrupt_in_endpoint == NULL) {
+ err("interrupt in endpoint not found");
+ goto error;
+ }
+ if (dev->interrupt_out_endpoint == NULL) {
+ err("interrupt out endpoint not found");
+ goto error;
+ }
+
+ in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
+ out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
+
+ dev->read_buffer_primary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_primary) {
+ err("Couldn't allocate read_buffer_primary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_primary, 'a', in_end_size);
+ memset(dev->read_buffer_primary + in_end_size, 'b', in_end_size);
+ memset(dev->read_buffer_primary + (2 * in_end_size), 'c', in_end_size);
+ memset(dev->read_buffer_primary + (3 * in_end_size), 'd', in_end_size);
+
+ dev->read_buffer_secondary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_secondary) {
+ err("Couldn't allocate read_buffer_secondary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_secondary, 'e', in_end_size);
+ memset(dev->read_buffer_secondary + in_end_size, 'f', in_end_size);
+ memset(dev->read_buffer_secondary + (2 * in_end_size), 'g', in_end_size);
+ memset(dev->read_buffer_secondary + (3 * in_end_size), 'h', in_end_size);
+
+ dev->interrupt_in_buffer = kmalloc(in_end_size, GFP_KERNEL);
+ if (!dev->interrupt_in_buffer) {
+ err("Couldn't allocate interrupt_in_buffer");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->interrupt_in_buffer, 'i', in_end_size);
+
+ dev->interrupt_in_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_in_urb) {
+ err("Couldn't allocate interrupt_in_urb");
+ goto error;
+ }
+ dev->interrupt_out_buffer = kmalloc(out_end_size, GFP_KERNEL);
+ if (!dev->interrupt_out_buffer) {
+ err("Couldn't allocate interrupt_out_buffer");
+ goto error;
+ }
+ dev->interrupt_out_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_out_urb) {
+ err("Couldn't allocate interrupt_out_urb");
+ goto error;
+ }
+
+ /* put dev in interface->private_data for disconnect */
+ udev->actconfig->interface[0].private_data = dev;
+
+ /* publish it */
+ minor_table[minor] = dev;
+
+ /* initialize the devfs node for this device and register it */
+ sprintf(name, "adutux%d", dev->minor);
+
+ dev->devfs = devfs_register(usb_devfs_handle, name,
+ DEVFS_FL_DEFAULT, USB_MAJOR,
+ ADU_MINOR_BASE + dev->minor,
+ S_IFCHR | S_IRUSR | S_IWUSR |
+ S_IRGRP | S_IWGRP | S_IROTH,
+ &adu_fops, NULL);
+
+ /* let the user know what node this device is now attached to */
+ info ("ADU%d now attached to /dev/usb/adutux%d", udev->descriptor.idProduct, dev->minor);
+
+ retval = dev;
+ /* unlock device */
+ up(&dev->sem);
+ goto unlock_minor_exit;
+
+error:
+ /* unlock device */
+ up(&dev->sem);
+ adu_delete(dev);
+ dev = NULL;
+
+unlock_minor_exit:
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2," %s : leave, return value 0x%.8lx (dev)", __FUNCTION__, (dev)?(long)dev:0);
+
+ return retval;
+}
+
+/**
+ * adu_disconnect
+ *
+ * Called by the usb core when the device is removed from the system.
+ */
+static void adu_disconnect(struct usb_device *udev, void *ptr)
+{
+ struct adu_device *dev;
+ int minor;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ dev = (struct adu_device *)ptr;
+
+ down (&minor_table_mutex);
+ down (&dev->sem);
+
+ minor = dev->minor;
+
+ /* remove our devfs node */
+ devfs_unregister(dev->devfs);
+
+ /* if the device is not opened, then we clean up right now */
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (!dev->open_count) {
+ up (&dev->sem);
+ adu_delete(dev);
+ } else {
+ dev->udev = NULL;
+ up (&dev->sem);
+ }
+
+ up (&minor_table_mutex);
+ info("ADU device adutux%d now disconnected", minor);
+
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_init
+ */
+static int __init adu_init(void)
+{
+ int result;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ /* register this driver with the USB subsystem */
+ result = usb_register(&adu_driver);
+ if (result < 0) {
+ err("usb_register failed for the "__FILE__" driver. "
+ "Error number %d", result);
+ goto exit;
+ }
+
+ info("adutux " DRIVER_DESC " " DRIVER_VERSION);
+ info("adutux is an experimental driver. Use at your own risk");
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, result);
+
+ return result;
+}
+
+/**
+ * adu_exit
+ */
+static void __exit adu_exit(void)
+{
+ dbg(2," %s : enter", __FUNCTION__);
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&adu_driver);
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+module_init(adu_init);
+module_exit(adu_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/Config.in linux-2.4.35.3.build/drivers/usb/Config.in
-- KERNELS/linux-2.4.35.3/drivers/usb/Config.in 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Config.in 2007-10-10 19:49:19.000000000 +0300
@@ -102,6 +102,7 @@ if [ "$CONFIG_USB" = "y" -o "$CONFIG_US

comment 'USB Miscellaneous drivers'
dep_tristate ' USB Diamond Rio500 support (EXPERIMENTAL)' CONFIG_USB_RIO500 $CONFIG_USB $CONFIG_EXPERIMENTAL
+ dep_tristate ' ADU devices from Ontrak Control Systems (EXPERIMENTAL)' CONFIG_USB_ADUTUX $CONFIG_USB $CONFIG_EXPERIMENTAL
dep_tristate ' Auerswald device support' CONFIG_USB_AUERSWALD $CONFIG_USB
dep_tristate ' Texas Instruments Graph Link USB (aka SilverLink) cable support' CONFIG_USB_TIGL $CONFIG_USB
dep_tristate ' Tieman Voyager USB Braille display support (EXPERIMENTAL)' CONFIG_USB_BRLVGER $CONFIG_USB $CONFIG_EXPERIMENTAL
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/Makefile linux-2.4.35.3.build/drivers/usb/Makefile
-- KERNELS/linux-2.4.35.3/drivers/usb/Makefile 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Makefile 2007-10-10 19:49:19.000000000 +0300
@@ -112,6 +112,7 @@ obj-$(CONFIG_USB_CATC) += catc.o
obj-$(CONFIG_USB_KAWETH) += kaweth.o
obj-$(CONFIG_USB_CDCETHER) += CDCEther.o
obj-$(CONFIG_USB_RIO500) += rio500.o
+obj-$(CONFIG_USB_ADUTUX) += adutux.o
obj-$(CONFIG_USB_TIGL) += tiglusb.o
obj-$(CONFIG_USB_DSBR) += dsbr100.o
obj-$(CONFIG_USB_MICROTEK) += microtek.o


2007-10-14 18:27:18

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Hello Vitaliy,

On Sun, Oct 14, 2007 at 08:37:25PM +0300, Vitaliy Ivanov wrote:
> Hello Willy, Greg and list,
>
> I have ported adutux driver for ADU series device list from 2.6 to 2.4.
> More on devices:
> http://www.ontrak.net/products.htm#Table%205
>
> Once I needed to make ADU200 work under 2.4 enterprise kernel and wasn't able to do this.
> My organization decided to use another device for our private purposes.
>
> I was always interested in kernel hacking and thought it's a good point to start it from.
> Just to add I'm not related to Ontrak in anyway. Also I'm not a Google person.
> Did it just for fun.
>
> A few technical notes:
> - I was trying to leave as much as possible adutux driver code from 2.6 as it's in the 2.6 mainline for a long time and seems like it works OK there.
> - Used one shot interrupt urbs that's why all intervals are 0.
> - Reused 67 minor number from 2.6 kernel for this device.
> - All 2.4 related changes are taken/reused from usb-skeleton.
>
> Patch is based on the latest 2.4.35.3.
>
> Performed a lot of tests but only under UHCI controller and ADU200 and all seems to be OK.
>
> I would like someone to perform code review as it's my first attempt to the kernel programming.
> Any comments, propositions are welcome.

At first glance, your backport looks clean. I have one comment however,
about the author and version. Since it's a backport from an existing
driver and not one you wrote yourself (eventhough you did the backporting
work), the MODULE_AUTHOR should not be changed (but I think you can add
yourself to it after a comma). The version should reflect the version you
derived it from, at least for bug tracking purposes.

Also, while I understand you would be very glad to get your work merged
(we all once had our first piece of code), I'd like to mention that you
seem to be the only user of this hardware under 2.4 (since it is currently
not supported). I'm not sure it's very reasonable to merge a driver in 2.4
right now for just one user. Even more, I understand that you finally moved
to other hardware, so my feeling is that you did this work as an exercice
(which was cleanly performed, BTW), but that it will not get any real use
in 2.4.

Since 2.4 is moving very slowly, there should be no problem applying
this patch to any version if you really need to use it. Maybe it would
even work with your 2.4 enterprise kernel.

Note that I'm not radically opposed to merge support for new drivers.
If you provide us with really good arguments for a merge, maybe I'll
change my opinion, but I doubt about it, since the only users of this
device must currently be running 2.6.

Thanks,
Willy

2007-10-14 20:45:47

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Willy,

On 10/14/07, Willy Tarreau <[email protected]> wrote:
> Hello Vitaliy,
>
> On Sun, Oct 14, 2007 at 08:37:25PM +0300, Vitaliy Ivanov wrote:
> > Hello Willy, Greg and list,
> >
> > I have ported adutux driver for ADU series device list from 2.6 to 2.4.
> > More on devices:
> > http://www.ontrak.net/products.htm#Table%205
> >
> > Once I needed to make ADU200 work under 2.4 enterprise kernel and wasn't able to do this.
> > My organization decided to use another device for our private purposes.
> >
> > I was always interested in kernel hacking and thought it's a good point to start it from.
> > Just to add I'm not related to Ontrak in anyway. Also I'm not a Google person.
> > Did it just for fun.
> >
> > A few technical notes:
> > - I was trying to leave as much as possible adutux driver code from 2.6 as it's in the 2.6 mainline for a long time and seems like it works OK there.
> > - Used one shot interrupt urbs that's why all intervals are 0.
> > - Reused 67 minor number from 2.6 kernel for this device.
> > - All 2.4 related changes are taken/reused from usb-skeleton.
> >
> > Patch is based on the latest 2.4.35.3.
> >
> > Performed a lot of tests but only under UHCI controller and ADU200 and all seems to be OK.
> >
> > I would like someone to perform code review as it's my first attempt to the kernel programming.
> > Any comments, propositions are welcome.
>
> At first glance, your backport looks clean. I have one comment however,
> about the author and version. Since it's a backport from an existing
> driver and not one you wrote yourself (eventhough you did the backporting
> work), the MODULE_AUTHOR should not be changed (but I think you can add
> yourself to it after a comma). The version should reflect the version you
> derived it from, at least for bug tracking purposes.

Sounds good for me. Will correct it.

>
> Also, while I understand you would be very glad to get your work merged
> (we all once had our first piece of code), I'd like to mention that you
> seem to be the only user of this hardware under 2.4 (since it is currently
> not supported). I'm not sure it's very reasonable to merge a driver in 2.4
> right now for just one user. Even more, I understand that you finally moved
> to other hardware, so my feeling is that you did this work as an exercice
> (which was cleanly performed, BTW), but that it will not get any real use
> in 2.4.

Yes, I would like it to be merged... But not just to see my name in
the kernel sources.
I'm the only one user of this hardware under 2.4 because it's some
kind of trick to make it work under 2.4 w/o its support in the kernel.
We moved to the other hardware because of our reasons and some
customers can move to the other OS where this hardware will be
supported.
I'm sure that we're not the first who tried to make it work in 2.4.
Original driver was created for 2.5 and > because interrupt out urbs
were not supported in 2.4. Now it's not an issue.

>
> Since 2.4 is moving very slowly, there should be no problem applying
> this patch to any version if you really need to use it. Maybe it would
> even work with your 2.4 enterprise kernel.
>
> Note that I'm not radically opposed to merge support for new drivers.
> If you provide us with really good arguments for a merge, maybe I'll
> change my opinion, but I doubt about it, since the only users of this
> device must currently be running 2.6.

I'm not going to force you to do this, also I can't do this:). But
after going through the recent news where Greg proposed to create
drivers for companies for free it looks really reasonable. I
understand that we are talking about 2.4 but if you will simply run
diff for adutux of 2.4 and 2.6 you will see that changes are really
trivial.
Also IMHO the more drivers are in the tree the more users will use it.
Once it will be merged in the mainline then it will be backported to
enterprise kernels and would gain wide usage.

Best,
Vitaliy

2007-10-14 22:40:59

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Sun, Oct 14, 2007 at 11:45:36PM +0300, Vitaliy Ivanov wrote:
> > Also, while I understand you would be very glad to get your work merged
> > (we all once had our first piece of code), I'd like to mention that you
> > seem to be the only user of this hardware under 2.4 (since it is currently
> > not supported). I'm not sure it's very reasonable to merge a driver in 2.4
> > right now for just one user. Even more, I understand that you finally moved
> > to other hardware, so my feeling is that you did this work as an exercice
> > (which was cleanly performed, BTW), but that it will not get any real use
> > in 2.4.
>
> Yes, I would like it to be merged... But not just to see my name in
> the kernel sources.
> I'm the only one user of this hardware under 2.4 because it's some
> kind of trick to make it work under 2.4 w/o its support in the kernel.
> We moved to the other hardware because of our reasons and some
> customers can move to the other OS where this hardware will be
> supported.

But something's strange. If people were using 2.4, this hardware has
never worked for them, right ? Why suddenly would they decide that
they have to switch OS or hardware ? Or maybe this hardware replaces
an old one which was supported ?

> I'm sure that we're not the first who tried to make it work in 2.4.

Perhaps, but if you were the last, interest is pretty limited :-)

> Original driver was created for 2.5 and > because interrupt out urbs
> were not supported in 2.4. Now it's not an issue.
>
> >
> > Since 2.4 is moving very slowly, there should be no problem applying
> > this patch to any version if you really need to use it. Maybe it would
> > even work with your 2.4 enterprise kernel.
> >
> > Note that I'm not radically opposed to merge support for new drivers.
> > If you provide us with really good arguments for a merge, maybe I'll
> > change my opinion, but I doubt about it, since the only users of this
> > device must currently be running 2.6.
>
> I'm not going to force you to do this, also I can't do this:). But
> after going through the recent news where Greg proposed to create
> drivers for companies for free it looks really reasonable. I
> understand that we are talking about 2.4 but if you will simply run
> diff for adutux of 2.4 and 2.6 you will see that changes are really
> trivial.

That's what I've seen. I can propose you something (unless someone
else raises his hand saying "no") : you update your patch with a
short description of what the hardware module is supposed to be used
for, and you accept to step up as the maintainer for this backport,
which will imply that you put your name and mail in the MAINTAINERS
file. That way, if you're the only user, nobody will be annoyed, and
if there are other users and some of them have problems, I don't waste
my time on something I don't know at all. If you agree with this deal
(which I think is fair), then I'm willing to merge your patch into
2.4.36-pre.

> Also IMHO the more drivers are in the tree the more users will use it.

Not necessarily. 2.4 is currently used by people who already are in 2.4
and cannot/do not want to switch, and by people who are looking for close
to zero maintenance. Drivers are often a reason to switch away from 2.4,
but not to stay in 2.4.

> Once it will be merged in the mainline then it will be backported to
> enterprise kernels and would gain wide usage.

I don't believe that. Enterprise kernels will not evolve much and will
probably not enable it as long as they have not tested it.

Regards,
Willy

2007-10-15 17:30:56

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Sun, 14 Oct 2007 23:45:36 +0300, "Vitaliy Ivanov" <[email protected]> wrote:

> Also IMHO the more drivers are in the tree the more users will use it.
> Once it will be merged in the mainline then it will be backported to
> enterprise kernels and would gain wide usage.

At least in case of RHEL, such backports never were automatic. In any
case, RHEL 2.1 and 3 do not receive new drivers anymore. We only do
bugfixes if something comes up. Realistically speaking, 2.4 kernels
are just too old for anyone to use. So, I think it would be best for
you to think in terms of Willy's tree only.

> + in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
> + out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);

Did you verify if this works? We use pre-swapped descriptors in 2.4.
I suspect you allocate 256 times more memory than necessary.

> +static void adu_delete(struct adu_device *dev)
> + kfree(dev);

> +static int adu_release_internal(struct adu_device *dev)
> + if (dev->udev == NULL) {
> + adu_delete(dev);

> +static int adu_open(struct inode *inode, struct file *file)
> + retval = adu_release_internal(dev);
> + up(&dev->sem);

The above very clearly is a use-after-free, in case the device was
open across a disconnect. Solution: Use minor_table_mutex to lock
dev->open_count instead of dev->sem. There's no rule that the lock
has to live inside the same structure with members it locks.

-- Pete

2007-10-15 20:05:09

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Pete,

On 10/15/07, Pete Zaitcev <[email protected]> wrote:
> On Sun, 14 Oct 2007 23:45:36 +0300, "Vitaliy Ivanov" <[email protected]> wrote:
>
> > Also IMHO the more drivers are in the tree the more users will use it.
> > Once it will be merged in the mainline then it will be backported to
> > enterprise kernels and would gain wide usage.
>
> At least in case of RHEL, such backports never were automatic. In any
> case, RHEL 2.1 and 3 do not receive new drivers anymore. We only do
> bugfixes if something comes up. Realistically speaking, 2.4 kernels
> are just too old for anyone to use. So, I think it would be best for
> you to think in terms of Willy's tree only.
>
> > + in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
> > + out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
>
> Did you verify if this works? We use pre-swapped descriptors in 2.4.
> I suspect you allocate 256 times more memory than necessary.
>
> > +static void adu_delete(struct adu_device *dev)
> > + kfree(dev);
>
> > +static int adu_release_internal(struct adu_device *dev)
> > + if (dev->udev == NULL) {
> > + adu_delete(dev);
>
> > +static int adu_open(struct inode *inode, struct file *file)
> > + retval = adu_release_internal(dev);
> > + up(&dev->sem);
>
> The above very clearly is a use-after-free, in case the device was
> open across a disconnect. Solution: Use minor_table_mutex to lock
> dev->open_count instead of dev->sem. There's no rule that the lock
> has to live inside the same structure with members it locks.

Thanks for your notes. Will check and correct it asap.

Vitaliy

2007-10-16 13:49:17

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Pete,

On Mon, 2007-10-15 at 20:30, Pete Zaitcev wrote:

> > + in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
> > + out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
>
> Did you verify if this works? We use pre-swapped descriptors in 2.4.
> I suspect you allocate 256 times more memory than necessary.

Just checked. Seems to be OK. At least printk shows shows it.

>
> > +static void adu_delete(struct adu_device *dev)
> > + kfree(dev);
>
> > +static int adu_release_internal(struct adu_device *dev)
> > + if (dev->udev == NULL) {
> > + adu_delete(dev);
>
> > +static int adu_open(struct inode *inode, struct file *file)
> > + retval = adu_release_internal(dev);
> > + up(&dev->sem);
>
> The above very clearly is a use-after-free, in case the device was
> open across a disconnect. Solution: Use minor_table_mutex to lock
> dev->open_count instead of dev->sem. There's no rule that the lock
> has to live inside the same structure with members it locks.

Yeah. You are right. Found similar issue in adu_release also.
It's a problem with 2.6 kernel driver.
So, I've got a material to create some fixes in 2.6 driver too.
I've reworked the code to avoid this issue.

Sending final patch as a reply to Willy's mail. Please check it.


Vitaliy

2007-10-16 13:58:18

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Willy,

On Mon, 2007-10-15 at 01:39, Willy Tarreau wrote:

> That's what I've seen. I can propose you something (unless someone
> else raises his hand saying "no") : you update your patch with a
> short description of what the hardware module is supposed to be used
> for, and you accept to step up as the maintainer for this backport,
> which will imply that you put your name and mail in the MAINTAINERS
> file. That way, if you're the only user, nobody will be annoyed, and
> if there are other users and some of them have problems, I don't waste
> my time on something I don't know at all. If you agree with this deal
> (which I think is fair), then I'm willing to merge your patch into
> 2.4.36-pre.

It's completely fair. I spent some time on lkml and I liked it.
I've a device and I can check/correct any issue we'll have with it(hope there won't be any;)).
So it's OK for me to be a maintainer for this driver.

Here is final patch with all issues corrected.

Again, comments are welcomed.

Vitaliy

--

adutux is a simple Linux device driver for ADU boards from Ontrak Control Systems.
The adutux driver exposes standard open/close/read/write API's to the user application.


Signed-off-by: Vitaliy Ivanov <[email protected]>
Tested-by: Vitaliy Ivanov <[email protected]>

--
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/Documentation/Configure.help linux-2.4.35.3.build/Documentation/Configure.help
--- KERNELS/linux-2.4.35.3/Documentation/Configure.help 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/Documentation/Configure.help 2007-10-14 19:19:06.000000000 +0300
@@ -16123,6 +16123,17 @@ CONFIG_USB_RIO500
The module will be called rio500.o. If you want to compile it as
a module, say M here and read <file:Documenatation/modules.txt>.

+Ontrak Control Systems ADU devices support
+CONFIG_USB_ADUTUX
+ Say Y here if you want to connect ADU 100/200 series devices to your
+ computer's USB port.
+ Details at: <http://www.ontrak.net/products.htm#Table%205>
+
+ This code is also available as a module ( = code which can be
+ inserted in and removed from the running kernel whenever you want).
+ The module will be called adutux.o. If you want to compile it as
+ a module, say M here and read <file:Documenatation/modules.txt>.
+
Auerswald device support
CONFIG_USB_AUERSWALD
Say Y here if you want to connect an Auerswald USB ISDN Device
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/adutux.c linux-2.4.35.3.build/drivers/usb/adutux.c
--- KERNELS/linux-2.4.35.3/drivers/usb/adutux.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/adutux.c 2007-10-16 16:27:41.000000000 +0300
@@ -0,0 +1,1043 @@
+/*
+ * This is a 2.4.* kernel version of adutux driver that
+ * was originaly created by John Homppi for 2.6.* kernels.
+ *
+ * Copyright (c) 2007 Vitaliy Ivanov ([email protected])
+ *
+ * Original note:
+ *
+ * adutux - driver for ADU devices from Ontrak Control Systems
+ * This is an experimental driver. Use at your own risk.
+ * This driver is not supported by Ontrak Control Systems.
+ *
+ * Copyright (c) 2003 John Homppi (SCO, leave this notice here)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * derived from the Lego USB Tower driver 0.56:
+ * Copyright (c) 2003 David Glance <[email protected]>
+ * 2001 Juergen Stuber <[email protected]>
+ * that was derived from USB Skeleton driver - 0.5
+ * Copyright (c) 2001 Greg Kroah-Hartman ([email protected])
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/devfs_fs_kernel.h>
+#include <linux/usb.h>
+#include <asm/uaccess.h>
+
+#ifdef CONFIG_USB_DEBUG
+static int debug = 5;
+#else
+static int debug = 1;
+#endif
+
+/* Use our own dbg macro */
+#undef dbg
+#define dbg(lvl, format, arg...) \
+do { \
+ if (debug >= lvl) \
+ printk(KERN_DEBUG __FILE__ " : " format " \n", ## arg); \
+} while (0)
+
+
+/* Version Information */
+#define DRIVER_VERSION "v0.0.13"
+#define DRIVER_AUTHOR "John Homppi, Vitaliy Ivanov"
+#define DRIVER_DESC "adutux (see http://www.ontrak.net)"
+
+/* Module parameters */
+module_param(debug, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Debug enabled or not");
+
+/* Define these values to match your device */
+#define ADU_VENDOR_ID 0x0a07
+#define ADU_PRODUCT_ID 0x0064
+
+/* table of devices that work with this driver */
+static struct usb_device_id device_table [] = {
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID) }, /* ADU100 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+20) }, /* ADU120 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+30) }, /* ADU130 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+100) }, /* ADU200 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+108) }, /* ADU208 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+118) }, /* ADU218 */
+ { }/* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+#define ADU_MINOR_BASE 67
+
+/* we can have up to this number of device plugged in at once */
+#define MAX_DEVICES 16
+
+#define COMMAND_TIMEOUT (2*HZ) /* 60 second timeout for a command */
+
+/* Structure to hold all of our device specific stuff */
+struct adu_device {
+ struct semaphore sem; /* locks this structure */
+ struct usb_device* udev; /* save off the usb device pointer */
+ //struct usb_interface* interface;
+ devfs_handle_t devfs; /* devfs device node */
+ unsigned char minor; /* the starting minor number for this device */
+
+ int open_count; /* number of times this port has been opened */
+
+ char* read_buffer_primary;
+ int read_buffer_length;
+ char* read_buffer_secondary;
+ int secondary_head;
+ int secondary_tail;
+ spinlock_t buflock;
+
+ wait_queue_head_t read_wait;
+ wait_queue_head_t write_wait;
+
+ char* interrupt_in_buffer;
+ struct usb_endpoint_descriptor* interrupt_in_endpoint;
+ struct urb* interrupt_in_urb;
+ int read_urb_finished;
+
+ char* interrupt_out_buffer;
+ struct usb_endpoint_descriptor* interrupt_out_endpoint;
+ struct urb* interrupt_out_urb;
+};
+
+/* the global usb devfs handle */
+extern devfs_handle_t usb_devfs_handle;
+
+/* 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 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_interrupt_in_callback(struct urb *urb);
+static void adu_interrupt_out_callback(struct urb *urb);
+static void *adu_probe(struct usb_device *dev, unsigned int ifnum, const struct usb_device_id *id);
+static void adu_disconnect(struct usb_device *dev, void *ptr);
+
+/* array of pointers to our devices that are currently connected */
+static struct adu_device *minor_table[MAX_DEVICES];
+
+/* lock to protect the minor_table structure */
+static DECLARE_MUTEX(minor_table_mutex);
+
+/* file operations needed when we register this driver */
+static struct file_operations adu_fops = {
+ owner: THIS_MODULE,
+ read: adu_read,
+ write: adu_write,
+ open: adu_open,
+ release: adu_release,
+};
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver adu_driver = {
+ name: "adutux",
+ probe: adu_probe,
+ disconnect: adu_disconnect,
+ fops: &adu_fops,
+ minor: ADU_MINOR_BASE,
+ id_table: device_table,
+};
+
+/**
+ * adu_debug_data
+ */
+static 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");
+}
+
+/**
+ * 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;
+ }
+
+ /* shutdown transfer */
+ usb_unlink_urb(dev->interrupt_in_urb);
+ usb_unlink_urb(dev->interrupt_out_urb);
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_delete
+ */
+static void adu_delete(struct adu_device *dev)
+{
+ dbg(2, "%s enter", __FUNCTION__);
+
+ minor_table[dev->minor] = NULL;
+ adu_abort_transfers(dev);
+
+ /* 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);
+ }
+ if (dev->read_buffer_primary != NULL) {
+ kfree(dev->read_buffer_primary);
+ }
+ if (dev->read_buffer_secondary != NULL) {
+ kfree(dev->read_buffer_secondary);
+ }
+ if (dev->interrupt_in_buffer != NULL) {
+ kfree(dev->interrupt_in_buffer);
+ }
+ if (dev->interrupt_out_buffer != NULL) {
+ kfree(dev->interrupt_out_buffer);
+ }
+ kfree(dev);
+
+ dbg(2, "%s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_interrupt_in_callback
+ */
+static void adu_interrupt_in_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+
+ spin_lock(&dev->buflock);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) && (urb->status != -ECONNRESET) &&
+ (urb->status != -ESHUTDOWN)) {
+ dbg(1," %s : nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ if (urb->actual_length > 0 && dev->interrupt_in_buffer[0] != 0x00) {
+ if (dev->read_buffer_length <
+ (4 * le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize)) -
+ (urb->actual_length)) {
+ memcpy (dev->read_buffer_primary +
+ dev->read_buffer_length,
+ dev->interrupt_in_buffer, urb->actual_length);
+
+ dev->read_buffer_length += urb->actual_length;
+ dbg(2," %s reading %d ", __FUNCTION__,
+ urb->actual_length);
+ } else {
+ dbg(1," %s : read_buffer overflow", __FUNCTION__);
+ }
+ }
+
+exit:
+ dev->read_urb_finished = 1;
+ spin_unlock(&dev->buflock);
+ /* always wake up so we recover from errors */
+ wake_up_interruptible(&dev->read_wait);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_interrupt_out_callback
+ */
+static void adu_interrupt_out_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5,__FUNCTION__, urb->actual_length, urb->transfer_buffer);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) &&
+ (urb->status != -ECONNRESET)) {
+ dbg(1, " %s :nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ wake_up_interruptible(&dev->write_wait);
+exit:
+
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_open
+ */
+static int adu_open(struct inode *inode, struct file *file)
+{
+ struct adu_device *dev = NULL;
+ int subminor;
+ int retval = 0;
+
+ dbg(2,"%s : enter", __FUNCTION__);
+
+ subminor = MINOR(inode->i_rdev) - ADU_MINOR_BASE;
+ if ((subminor < 0) ||
+ (subminor >= MAX_DEVICES)) {
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our minor table and get our local data for this minor */
+ down(&minor_table_mutex);
+ dev = minor_table[subminor];
+ if (dev == NULL) {
+ retval = -ENODEV;
+ goto unlock_exit;
+ }
+
+ /* lock this device */
+ down(&dev->sem);
+
+ /* increment our usage count for the device */
+ ++dev->open_count;
+ dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
+
+ /* check that nobody else is using the device */
+ /* NOTE: the cleanup code will decrement the count to 1 */
+ if (dev->open_count > 1) {
+ retval = -EBUSY;
+ /* unlock this device */
+ up(&dev->sem);
+ goto error;
+ }
+
+ /* save device in the file's private structure */
+ file->private_data = dev;
+
+ /* initialize in direction */
+ dev->read_buffer_length = 0;
+
+ /* fixup first read by having urb waiting for it */
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev, dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ if (retval != 0) {
+ err("Couldn't submit interrupt_in_urb");
+ /* we ignore failure, just notify about it */
+ /*goto error;*/
+ }
+ /* end of fixup for first read */
+
+ /* unlock this device */
+ up(&dev->sem);
+ goto unlock_exit;
+
+error:
+ adu_release_internal(dev);
+
+unlock_exit:
+ /* unlock the minor table */
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2, " %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_release_internal
+ */
+static int adu_release_internal(struct adu_device *dev)
+{
+ int retval = 0;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ if (dev->udev == NULL) {
+ /* the device was unplugged before the file was released */
+ adu_delete(dev);
+ goto exit;
+ }
+
+ /* lock this device */
+ down(&dev->sem);
+
+ /* decrement our usage count for the device */
+ --dev->open_count;
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (dev->open_count <= 0) {
+ adu_abort_transfers(dev);
+ dev->open_count = 0;
+ }
+
+ /* unlock this device */
+ up(&dev->sem);
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+ return retval;
+}
+
+/**
+ * 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;
+ }
+
+ dev = file->private_data;
+
+ if (dev == NULL) {
+ dbg(1," %s : object is NULL", __FUNCTION__);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our minor table */
+ down(&minor_table_mutex);
+
+ /* lock our device */
+ down(&dev->sem);
+
+ if (dev->open_count <= 0) {
+ dbg(1," %s : device not opened", __FUNCTION__);
+ up(&dev->sem);
+ up(&minor_table_mutex);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* unlocking device */
+ up(&dev->sem);
+
+ /* do the work */
+ retval = adu_release_internal(dev);
+
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_read
+ */
+static ssize_t adu_read(struct file *file, __user 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 = %Zd, file=%p", __FUNCTION__, count, file);
+
+ dev = file->private_data;
+ dbg(2," %s : dev=%p", __FUNCTION__, dev);
+ /* lock this object */
+ if (down_interruptible(&dev->sem))
+ return -ERESTARTSYS;
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ 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);
+ if (i < 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+ 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);
+ /* we have a free buffer so use it */
+ should_submit = 1;
+ } 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;
+
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev, dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ 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) {
+ FILL_INT_URB(dev->interrupt_in_urb,
+ dev->udev,
+ usb_rcvintpipe(dev->udev,dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ usb_submit_urb(dev->interrupt_in_urb);
+ /* we ignore failure */
+ }
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_write
+ */
+static ssize_t adu_write(struct file *file, const __user char *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct adu_device *dev;
+ size_t bytes_written = 0;
+ size_t bytes_to_write;
+ size_t buffer_size;
+ int retval = 0;
+ int timeout = 0;
+
+ dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
+
+ dev = file->private_data;
+
+ /* lock this object */
+ down_interruptible(&dev->sem);
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ retval = -ENODEV;
+ err("No device or device unplugged %d", retval);
+ goto exit;
+ }
+
+ /* verify that we actually have some data to write */
+ if (count == 0) {
+ dbg(1," %s : write request of 0 bytes", __FUNCTION__);
+ goto exit;
+ }
+
+
+ while (count > 0) {
+ if (dev->interrupt_out_urb->status == -EINPROGRESS) {
+ timeout = COMMAND_TIMEOUT;
+
+ while (timeout > 0) {
+ if (signal_pending(current)) {
+ dbg(1," %s : interrupted", __FUNCTION__);
+ retval = -EINTR;
+ goto exit;
+ }
+ up(&dev->sem);
+ timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ down_interruptible(&dev->sem);
+ if (timeout > 0) {
+ break;
+ }
+ dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
+ }
+
+
+ dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
+
+ if (timeout == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit;
+ }
+
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+
+ } else {
+ dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
+
+ /* write the data into interrupt_out_buffer from userspace */
+ buffer_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
+ bytes_to_write = count > buffer_size ? buffer_size : count;
+ dbg(4," %s : buffer_size = %Zd, count = %Zd, bytes_to_write = %Zd",
+ __FUNCTION__, buffer_size, count, bytes_to_write);
+
+ if (copy_from_user(dev->interrupt_out_buffer, buffer, bytes_to_write) != 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ /* send off the urb */
+ FILL_INT_URB(
+ dev->interrupt_out_urb,
+ dev->udev,
+ usb_sndintpipe(dev->udev, dev->interrupt_out_endpoint->bEndpointAddress),
+ dev->interrupt_out_buffer,
+ bytes_to_write,
+ adu_interrupt_out_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_urb->actual_length = bytes_to_write;
+ retval = usb_submit_urb(dev->interrupt_out_urb);
+ if (retval < 0) {
+ err("Couldn't submit interrupt_out_urb %d", retval);
+ goto exit;
+ }
+
+ buffer += bytes_to_write;
+ count -= bytes_to_write;
+
+ bytes_written += bytes_to_write;
+ }
+ }
+
+ retval = bytes_written;
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_probe
+ *
+ * Called by the usb core when a new device is connected that it thinks
+ * this driver might be interested in.
+ */
+static void *adu_probe(struct usb_device *udev,
+ unsigned int ifnum, const struct usb_device_id *id)
+{
+ struct adu_device *dev = NULL;
+ struct usb_interface *interface;
+ struct usb_interface_descriptor *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ int minor;
+ int in_end_size;
+ int out_end_size;
+ int i;
+ char name[10];
+ void *retval = NULL;
+
+ dbg(2, " %s : enter", __FUNCTION__);
+
+ if (udev == NULL) {
+ info ("udev is NULL.");
+ goto exit;
+ }
+
+ if (ifnum != 0) {
+ info ("Strange interface number %d.", ifnum);
+ goto exit;
+ }
+
+ /* select a "subminor" number (part of a minor number) */
+ down (&minor_table_mutex);
+ for (minor = 0; minor < MAX_DEVICES; ++minor) {
+ if (minor_table[minor] == NULL)
+ break;
+ }
+ if (minor >= MAX_DEVICES) {
+ info ("Too many devices plugged in, can not handle this device.");
+ goto unlock_minor_exit;
+ }
+
+ /* 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 unlock_minor_exit;
+ }
+ memset (dev, 0x00, sizeof (*dev));
+
+ init_MUTEX(&dev->sem);
+ spin_lock_init(&dev->buflock);
+ down(&dev->sem);
+ dev->udev = udev;
+
+ interface = &dev->udev->actconfig->interface[0];
+ iface_desc = &interface->altsetting[0];
+
+ //dev->interface = interface;
+ dev->minor = minor;
+ 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->interrupt_out_buffer = NULL;
+ dev->interrupt_out_endpoint = NULL;
+ dev->interrupt_out_urb = NULL;
+
+ dev->read_urb_finished = 0;
+
+ /* set up the endpoint information */
+ for (i = 0; i < iface_desc->bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i];
+
+ /**
+ * Check if the endpoint has IN direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_IN) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt in endpoint */
+ dev->interrupt_in_endpoint = endpoint;
+ }
+
+ /**
+ * Check if the endpoint has OUT direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt out endpoint */
+ dev->interrupt_out_endpoint = endpoint;
+ }
+ }
+
+ if(dev->interrupt_in_endpoint == NULL) {
+ err("interrupt in endpoint not found");
+ goto error;
+ }
+ if (dev->interrupt_out_endpoint == NULL) {
+ err("interrupt out endpoint not found");
+ goto error;
+ }
+
+ in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
+ out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
+
+ dev->read_buffer_primary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_primary) {
+ err("Couldn't allocate read_buffer_primary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_primary, 'a', in_end_size);
+ memset(dev->read_buffer_primary + in_end_size, 'b', in_end_size);
+ memset(dev->read_buffer_primary + (2 * in_end_size), 'c', in_end_size);
+ memset(dev->read_buffer_primary + (3 * in_end_size), 'd', in_end_size);
+
+ dev->read_buffer_secondary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_secondary) {
+ err("Couldn't allocate read_buffer_secondary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_secondary, 'e', in_end_size);
+ memset(dev->read_buffer_secondary + in_end_size, 'f', in_end_size);
+ memset(dev->read_buffer_secondary + (2 * in_end_size), 'g', in_end_size);
+ memset(dev->read_buffer_secondary + (3 * in_end_size), 'h', in_end_size);
+
+ dev->interrupt_in_buffer = kmalloc(in_end_size, GFP_KERNEL);
+ if (!dev->interrupt_in_buffer) {
+ err("Couldn't allocate interrupt_in_buffer");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->interrupt_in_buffer, 'i', in_end_size);
+
+ dev->interrupt_in_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_in_urb) {
+ err("Couldn't allocate interrupt_in_urb");
+ goto error;
+ }
+ dev->interrupt_out_buffer = kmalloc(out_end_size, GFP_KERNEL);
+ if (!dev->interrupt_out_buffer) {
+ err("Couldn't allocate interrupt_out_buffer");
+ goto error;
+ }
+ dev->interrupt_out_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_out_urb) {
+ err("Couldn't allocate interrupt_out_urb");
+ goto error;
+ }
+
+ /* put dev in interface->private_data for disconnect */
+ udev->actconfig->interface[0].private_data = dev;
+
+ /* publish it */
+ minor_table[minor] = dev;
+
+ /* initialize the devfs node for this device and register it */
+ sprintf(name, "adutux%d", dev->minor);
+
+ dev->devfs = devfs_register(usb_devfs_handle, name,
+ DEVFS_FL_DEFAULT, USB_MAJOR,
+ ADU_MINOR_BASE + dev->minor,
+ S_IFCHR | S_IRUSR | S_IWUSR |
+ S_IRGRP | S_IWGRP | S_IROTH,
+ &adu_fops, NULL);
+
+ /* let the user know what node this device is now attached to */
+ info ("ADU%d now attached to /dev/usb/adutux%d", udev->descriptor.idProduct, dev->minor);
+
+ retval = dev;
+ /* unlock device */
+ up(&dev->sem);
+ goto unlock_minor_exit;
+
+error:
+ /* unlock device */
+ up(&dev->sem);
+ adu_delete(dev);
+ dev = NULL;
+
+unlock_minor_exit:
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2," %s : leave, return value 0x%.8lx (dev)", __FUNCTION__, (dev)?(long)dev:0);
+
+ return retval;
+}
+
+/**
+ * adu_disconnect
+ *
+ * Called by the usb core when the device is removed from the system.
+ */
+static void adu_disconnect(struct usb_device *udev, void *ptr)
+{
+ struct adu_device *dev;
+ int minor;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ dev = (struct adu_device *)ptr;
+
+ down (&minor_table_mutex);
+ down (&dev->sem);
+
+ minor = dev->minor;
+
+ /* remove our devfs node */
+ devfs_unregister(dev->devfs);
+
+ /* if the device is not opened, then we clean up right now */
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (!dev->open_count) {
+ up (&dev->sem);
+ adu_delete(dev);
+ } else {
+ dev->udev = NULL;
+ up (&dev->sem);
+ }
+
+ up (&minor_table_mutex);
+ info("ADU device adutux%d now disconnected", minor);
+
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_init
+ */
+static int __init adu_init(void)
+{
+ int result;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ /* register this driver with the USB subsystem */
+ result = usb_register(&adu_driver);
+ if (result < 0) {
+ err("usb_register failed for the "__FILE__" driver. "
+ "Error number %d", result);
+ goto exit;
+ }
+
+ info("adutux " DRIVER_DESC " " DRIVER_VERSION);
+ info("adutux is an experimental driver. Use at your own risk");
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, result);
+
+ return result;
+}
+
+/**
+ * adu_exit
+ */
+static void __exit adu_exit(void)
+{
+ dbg(2," %s : enter", __FUNCTION__);
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&adu_driver);
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+module_init(adu_init);
+module_exit(adu_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/Config.in linux-2.4.35.3.build/drivers/usb/Config.in
--- KERNELS/linux-2.4.35.3/drivers/usb/Config.in 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Config.in 2007-10-10 19:49:19.000000000 +0300
@@ -102,6 +102,7 @@ if [ "$CONFIG_USB" = "y" -o "$CONFIG_US

comment 'USB Miscellaneous drivers'
dep_tristate ' USB Diamond Rio500 support (EXPERIMENTAL)' CONFIG_USB_RIO500 $CONFIG_USB $CONFIG_EXPERIMENTAL
+ dep_tristate ' ADU devices from Ontrak Control Systems (EXPERIMENTAL)' CONFIG_USB_ADUTUX $CONFIG_USB $CONFIG_EXPERIMENTAL
dep_tristate ' Auerswald device support' CONFIG_USB_AUERSWALD $CONFIG_USB
dep_tristate ' Texas Instruments Graph Link USB (aka SilverLink) cable support' CONFIG_USB_TIGL $CONFIG_USB
dep_tristate ' Tieman Voyager USB Braille display support (EXPERIMENTAL)' CONFIG_USB_BRLVGER $CONFIG_USB $CONFIG_EXPERIMENTAL
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/drivers/usb/Makefile linux-2.4.35.3.build/drivers/usb/Makefile
--- KERNELS/linux-2.4.35.3/drivers/usb/Makefile 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Makefile 2007-10-10 19:49:19.000000000 +0300
@@ -112,6 +112,7 @@ obj-$(CONFIG_USB_CATC) += catc.o
obj-$(CONFIG_USB_KAWETH) += kaweth.o
obj-$(CONFIG_USB_CDCETHER) += CDCEther.o
obj-$(CONFIG_USB_RIO500) += rio500.o
+obj-$(CONFIG_USB_ADUTUX) += adutux.o
obj-$(CONFIG_USB_TIGL) += tiglusb.o
obj-$(CONFIG_USB_DSBR) += dsbr100.o
obj-$(CONFIG_USB_MICROTEK) += microtek.o
diff -uprN -X dontdiff KERNELS/linux-2.4.35.3/MAINTAINERS linux-2.4.35.3.build/MAINTAINERS
--- KERNELS/linux-2.4.35.3/MAINTAINERS 2007-09-24 01:02:58.000000000 +0300
+++ linux-2.4.35.3.build/MAINTAINERS 2007-10-16 16:31:14.000000000 +0300
@@ -173,6 +173,11 @@ M: Thorsten Knabe <linux@thorsten-knabe.
W: http://linux.thorsten-knabe.de
S: Maintained

+ADUTUX ONTRAK CONTROL SYSTEMS USB DRIVER
+P: Vitaliy Ivanov
+M: [email protected]
+S: Maintained
+
ADVANSYS SCSI DRIVER
P: Bob Frey
M: [email protected]


2007-10-16 14:55:32

by Greg KH

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, Oct 16, 2007 at 04:48:54PM +0300, Vitaliy Ivanov wrote:
> Pete,
>
> On Mon, 2007-10-15 at 20:30, Pete Zaitcev wrote:
>
> > > + in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
> > > + out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
> >
> > Did you verify if this works? We use pre-swapped descriptors in 2.4.
> > I suspect you allocate 256 times more memory than necessary.
>
> Just checked. Seems to be OK. At least printk shows shows it.

That's probably because you tested this on a little-endian machine :)

Pete is right, this code is incorrect for 2.4, drop the le16_to_cpu
function, the wMaxPacketSize variable is in native-endian form in 2.4
and early 2.6 versions.

thanks,

greg k-h

2007-10-16 15:45:31

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, Oct 16, 2007 at 04:54:49PM +0300, Vitaliy Ivanov wrote:
> Willy,
>
> On Mon, 2007-10-15 at 01:39, Willy Tarreau wrote:
>
> > That's what I've seen. I can propose you something (unless someone
> > else raises his hand saying "no") : you update your patch with a
> > short description of what the hardware module is supposed to be used
> > for, and you accept to step up as the maintainer for this backport,
> > which will imply that you put your name and mail in the MAINTAINERS
> > file. That way, if you're the only user, nobody will be annoyed, and
> > if there are other users and some of them have problems, I don't waste
> > my time on something I don't know at all. If you agree with this deal
> > (which I think is fair), then I'm willing to merge your patch into
> > 2.4.36-pre.
>
> It's completely fair. I spent some time on lkml and I liked it.
> I've a device and I can check/correct any issue we'll have with it(hope there won't be any;)).
> So it's OK for me to be a maintainer for this driver.
>
> Here is final patch with all issues corrected.
>
> Again, comments are welcomed.

OK, I have no objection, but please apply the fixes the le16 problem as
suggested by Pete and Greg first. Also, you will probably receive more
comments, and/or criticisms from further reviews. This is normal and
expected. You just have to fix your code so that it can be merged.

> adutux is a simple Linux device driver for ADU boards from Ontrak Control Systems.
> The adutux driver exposes standard open/close/read/write API's to the user application.

If this is going to be the commit message, please reduce lines to less
than 70 chars, and also enumerate the supportd devices and the one you
performed the tests with. It helps a lot when users encounter problems.

Also, if you did not (I forgot to check), please ensure that the adutux
maintainer in 2.6 is CC'd.

Last but not least, please remove the "KERNELS/" path component from
your next diff.

Thanks,
Willy

2007-10-16 17:52:58

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, 16 Oct 2007 16:54:49 +0300, Vitaliy Ivanov <[email protected]> wrote:

> Again, comments are welcomed.

It looks like you misunderstood why a static lock protects open counts.
This is done so you do not need to worry about in-structure lock which
can be freed together with the structure. Look at this:

> +static int adu_release_internal(struct adu_device *dev)
> +{
> + /* lock this device */
> + down(&dev->sem);
> + /* decrement our usage count for the device */
> + --dev->open_count;
> + if (dev->open_count <= 0) {
> + adu_abort_transfers(dev);
> + dev->open_count = 0;
> + }
> + /* unlock this device */
> + up(&dev->sem);

The dev->sem is entirely unnecessary here. Every time you use
open_count, it's protected by minor_table_mutex. The name is a litte
unfortunate, feel free to rename it.

This is probably a problem in 2.6 as well. I don't know why people keep
writing these things. Someone at Ontrak needs to look into it.

-- Pete

2007-10-16 17:57:48

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, 16 Oct 2007 17:41:38 +0200, Willy Tarreau <[email protected]> wrote:

> OK, I have no objection, but please apply the fixes the le16 problem as
> suggested by Pete and Greg first. Also, you will probably receive more
> comments, and/or criticisms from further reviews. This is normal and
> expected. You just have to fix your code so that it can be merged.

I do not object to the driver (with byte order fixed), although it seems
written oddly... E.g. lots of trivial comments, the open count locked
by wrong lock. But I do not want to hold the 2.4 version hostage to
errors of the mainline. We should've caught them when we merged adutux
into Greg's tree. I have to admit I'm skipping reviewing a lot of 2.6.

-- Pete

2007-10-16 18:28:18

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, Oct 16, 2007 at 10:56:03AM -0700, Pete Zaitcev wrote:
> On Tue, 16 Oct 2007 17:41:38 +0200, Willy Tarreau <[email protected]> wrote:
>
> > OK, I have no objection, but please apply the fixes the le16 problem as
> > suggested by Pete and Greg first. Also, you will probably receive more
> > comments, and/or criticisms from further reviews. This is normal and
> > expected. You just have to fix your code so that it can be merged.
>
> I do not object to the driver (with byte order fixed), although it seems
> written oddly... E.g. lots of trivial comments, the open count locked
> by wrong lock. But I do not want to hold the 2.4 version hostage to
> errors of the mainline. We should've caught them when we merged adutux
> into Greg's tree. I have to admit I'm skipping reviewing a lot of 2.6.

I really appreciate your position, Pete. It's fair and motivating for
newcomers. I hope Vitalyi will be able to forward-port some fixes and
cleanups to 2.6 in the future.

Thanks,
Willy

2007-10-16 18:28:37

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, 2007-10-16 at 20:56, Pete Zaitcev wrote:
> On Tue, 16 Oct 2007 17:41:38 +0200, Willy Tarreau <[email protected]> wrote:
>
> > OK, I have no objection, but please apply the fixes the le16 problem as
> > suggested by Pete and Greg first. Also, you will probably receive more
> > comments, and/or criticisms from further reviews. This is normal and
> > expected. You just have to fix your code so that it can be merged.
>
> I do not object to the driver (with byte order fixed), although it seems
> written oddly... E.g. lots of trivial comments, the open count locked
> by wrong lock. But I do not want to hold the 2.4 version hostage to
> errors of the mainline. We should've caught them when we merged adutux
> into Greg's tree. I have to admit I'm skipping reviewing a lot of 2.6.

No Pete. Seems it's my changes. Actually such approach is proposed by
usb-skeleton in 2.4. Will rework code to lock it correctly and clean
code from trivial comments.

Will back to this soon.

Vitaliy

2007-10-16 18:28:58

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Tue, 2007-10-16 at 18:41, Willy Tarreau wrote:

> OK, I have no objection, but please apply the fixes the le16 problem as
> suggested by Pete and Greg first. Also, you will probably receive more
> comments, and/or criticisms from further reviews. This is normal and
> expected. You just have to fix your code so that it can be merged.

Done. Removed all le16_to_cpu's.
Comments are OK. Just a little bit busy on my regular work. So can give
responses not so quickly as you probably wanted.

> > adutux is a simple Linux device driver for ADU boards from Ontrak Control Systems.
> > The adutux driver exposes standard open/close/read/write API's to the user application.
>
> If this is going to be the commit message, please reduce lines to less
> than 70 chars, and also enumerate the supportd devices and the one you
> performed the tests with. It helps a lot when users encounter problems.
>

It's a driver description you asked for previously.

Commit message should look like this probably:
"Port of adutux driver from 2.6 kernel for Ontrak ADU boards."

Devices list:
As I said previously supported devices are:
http://www.ontrak.net/products.htm#Table%205
Just to list them here:
ADU100, ADU120, ADU130, ADU200, ADU208, ADU218.

Testing is performed on ADU200.
But command structure is identical for this devices and it is only
individual command syntax that varies between different ADU devices.

> Also, if you did not (I forgot to check), please ensure that the adutux
> maintainer in 2.6 is CC'd.

There is not maintainer for this device in 2.6.


Following Pete notes I will rework code and give it for review once again.
As I said it's usb-skeleton approach that I reused during the port.

Anyway, will get back to this soon.

Vitaliy

2007-10-17 18:21:12

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Willy & Pete,

On Tue, 2007-10-16 at 21:24, Vitaliy Ivanov wrote:
> Following Pete notes I will rework code and give it for review once again.
> As I said it's usb-skeleton approach that I reused during the port.
>
> Anyway, will get back to this soon.

Reworked code a little.

Issue with locks...
Now minor_table_mutex is used to protect the minor_table structure and nothing else.
It was its first purpose.

dev->sem is used to protect device manipulations. It's a normal practice in 2.4.
So I leave it this way. I think it's OK, Pete?

So the final patch, hope it's final, no?:)

P.S. Willy all the details you asked before can be found in my previous mail(description, device list, etc).


Signed-off-by: Vitaliy Ivanov <[email protected]>
Tested-by: Vitaliy Ivanov <[email protected]>

--

diff -uprN -X dontdiff linux-2.4.35.3/Documentation/Configure.help linux-2.4.35.3.build/Documentation/Configure.help
--- linux-2.4.35.3/Documentation/Configure.help 2007-10-16 20:52:09.000000000 +0300
+++ linux-2.4.35.3.build/Documentation/Configure.help 2007-10-14 19:19:06.000000000 +0300
@@ -16123,6 +16123,17 @@ CONFIG_USB_RIO500
The module will be called rio500.o. If you want to compile it as
a module, say M here and read <file:Documenatation/modules.txt>.

+Ontrak Control Systems ADU devices support
+CONFIG_USB_ADUTUX
+ Say Y here if you want to connect ADU 100/200 series devices to your
+ computer's USB port.
+ Details at: <http://www.ontrak.net/products.htm#Table%205>
+
+ This code is also available as a module ( = code which can be
+ inserted in and removed from the running kernel whenever you want).
+ The module will be called adutux.o. If you want to compile it as
+ a module, say M here and read <file:Documenatation/modules.txt>.
+
Auerswald device support
CONFIG_USB_AUERSWALD
Say Y here if you want to connect an Auerswald USB ISDN Device
diff -uprN -X dontdiff linux-2.4.35.3/drivers/usb/adutux.c linux-2.4.35.3.build/drivers/usb/adutux.c
--- linux-2.4.35.3/drivers/usb/adutux.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/adutux.c 2007-10-17 20:57:03.000000000 +0300
@@ -0,0 +1,1040 @@
+/*
+ * This is a 2.4.* kernel version of adutux driver that
+ * was originaly created by John Homppi for 2.6.* kernels.
+ *
+ * Copyright (c) 2007 Vitaliy Ivanov ([email protected])
+ *
+ * Original note:
+ *
+ * adutux - driver for ADU devices from Ontrak Control Systems
+ * This is an experimental driver. Use at your own risk.
+ * This driver is not supported by Ontrak Control Systems.
+ *
+ * Copyright (c) 2003 John Homppi (SCO, leave this notice here)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * derived from the Lego USB Tower driver 0.56:
+ * Copyright (c) 2003 David Glance <[email protected]>
+ * 2001 Juergen Stuber <[email protected]>
+ * that was derived from USB Skeleton driver - 0.5
+ * Copyright (c) 2001 Greg Kroah-Hartman ([email protected])
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/devfs_fs_kernel.h>
+#include <linux/usb.h>
+#include <asm/uaccess.h>
+
+#ifdef CONFIG_USB_DEBUG
+static int debug = 5;
+#else
+static int debug = 1;
+#endif
+
+/* Use our own dbg macro */
+#undef dbg
+#define dbg(lvl, format, arg...) \
+do { \
+ if (debug >= lvl) \
+ printk(KERN_DEBUG __FILE__ " : " format " \n", ## arg); \
+} while (0)
+
+
+/* Version Information */
+#define DRIVER_VERSION "v0.0.13"
+#define DRIVER_AUTHOR "John Homppi, Vitaliy Ivanov"
+#define DRIVER_DESC "adutux (see http://www.ontrak.net)"
+
+/* Module parameters */
+module_param(debug, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Debug enabled or not");
+
+/* Define these values to match your device */
+#define ADU_VENDOR_ID 0x0a07
+#define ADU_PRODUCT_ID 0x0064
+
+/* table of devices that work with this driver */
+static struct usb_device_id device_table [] = {
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID) }, /* ADU100 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+20) }, /* ADU120 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+30) }, /* ADU130 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+100) }, /* ADU200 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+108) }, /* ADU208 */
+ { USB_DEVICE(ADU_VENDOR_ID, ADU_PRODUCT_ID+118) }, /* ADU218 */
+ { }/* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+#define ADU_MINOR_BASE 67
+
+/* we can have up to this number of device plugged in at once */
+#define MAX_DEVICES 16
+
+#define COMMAND_TIMEOUT (2*HZ) /* 60 second timeout for a command */
+
+/* Structure to hold all of our device specific stuff */
+struct adu_device {
+ struct semaphore sem; /* locks this structure */
+ struct usb_device* udev; /* save off the usb device pointer */
+ //struct usb_interface* interface;
+ devfs_handle_t devfs; /* devfs device node */
+ unsigned char minor; /* the starting minor number for this device */
+
+ int open_count; /* number of times this port has been opened */
+
+ char* read_buffer_primary;
+ int read_buffer_length;
+ char* read_buffer_secondary;
+ int secondary_head;
+ int secondary_tail;
+ spinlock_t buflock;
+
+ wait_queue_head_t read_wait;
+ wait_queue_head_t write_wait;
+
+ char* interrupt_in_buffer;
+ struct usb_endpoint_descriptor* interrupt_in_endpoint;
+ struct urb* interrupt_in_urb;
+ int read_urb_finished;
+
+ char* interrupt_out_buffer;
+ struct usb_endpoint_descriptor* interrupt_out_endpoint;
+ struct urb* interrupt_out_urb;
+};
+
+/* the global usb devfs handle */
+extern devfs_handle_t usb_devfs_handle;
+
+/* 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 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_interrupt_in_callback(struct urb *urb);
+static void adu_interrupt_out_callback(struct urb *urb);
+static void *adu_probe(struct usb_device *dev, unsigned int ifnum, const struct usb_device_id *id);
+static void adu_disconnect(struct usb_device *dev, void *ptr);
+
+/* array of pointers to our devices that are currently connected */
+static struct adu_device *minor_table[MAX_DEVICES];
+
+/* lock to protect the minor_table structure */
+static DECLARE_MUTEX(minor_table_mutex);
+
+/* file operations needed when we register this driver */
+static struct file_operations adu_fops = {
+ owner: THIS_MODULE,
+ read: adu_read,
+ write: adu_write,
+ open: adu_open,
+ release: adu_release,
+};
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver adu_driver = {
+ name: "adutux",
+ probe: adu_probe,
+ disconnect: adu_disconnect,
+ fops: &adu_fops,
+ minor: ADU_MINOR_BASE,
+ id_table: device_table,
+};
+
+/**
+ * adu_debug_data
+ */
+static 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");
+}
+
+/**
+ * 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;
+ }
+
+ /* shutdown transfer */
+ usb_unlink_urb(dev->interrupt_in_urb);
+ usb_unlink_urb(dev->interrupt_out_urb);
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_delete
+ */
+static void adu_delete(struct adu_device *dev)
+{
+ dbg(2, "%s enter", __FUNCTION__);
+
+ minor_table[dev->minor] = NULL;
+ adu_abort_transfers(dev);
+
+ /* 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);
+ }
+ if (dev->read_buffer_primary != NULL) {
+ kfree(dev->read_buffer_primary);
+ }
+ if (dev->read_buffer_secondary != NULL) {
+ kfree(dev->read_buffer_secondary);
+ }
+ if (dev->interrupt_in_buffer != NULL) {
+ kfree(dev->interrupt_in_buffer);
+ }
+ if (dev->interrupt_out_buffer != NULL) {
+ kfree(dev->interrupt_out_buffer);
+ }
+ kfree(dev);
+
+ dbg(2, "%s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_interrupt_in_callback
+ */
+static void adu_interrupt_in_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+
+ spin_lock(&dev->buflock);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) && (urb->status != -ECONNRESET) &&
+ (urb->status != -ESHUTDOWN)) {
+ dbg(1," %s : nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ if (urb->actual_length > 0 && dev->interrupt_in_buffer[0] != 0x00) {
+ if (dev->read_buffer_length <
+ (4 * dev->interrupt_in_endpoint->wMaxPacketSize) -
+ (urb->actual_length)) {
+ memcpy (dev->read_buffer_primary +
+ dev->read_buffer_length,
+ dev->interrupt_in_buffer, urb->actual_length);
+
+ dev->read_buffer_length += urb->actual_length;
+ dbg(2," %s reading %d ", __FUNCTION__,
+ urb->actual_length);
+ } else {
+ dbg(1," %s : read_buffer overflow", __FUNCTION__);
+ }
+ }
+
+exit:
+ dev->read_urb_finished = 1;
+ spin_unlock(&dev->buflock);
+ /* always wake up so we recover from errors */
+ wake_up_interruptible(&dev->read_wait);
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_interrupt_out_callback
+ */
+static void adu_interrupt_out_callback(struct urb *urb)
+{
+ struct adu_device *dev = urb->context;
+
+ dbg(4," %s : enter, status %d", __FUNCTION__, urb->status);
+ adu_debug_data(5,__FUNCTION__, urb->actual_length, urb->transfer_buffer);
+
+ if (urb->status != 0) {
+ if ((urb->status != -ENOENT) &&
+ (urb->status != -ECONNRESET)) {
+ dbg(1, " %s :nonzero status received: %d",
+ __FUNCTION__, urb->status);
+ }
+ goto exit;
+ }
+
+ wake_up_interruptible(&dev->write_wait);
+exit:
+
+ adu_debug_data(5, __FUNCTION__, urb->actual_length,
+ urb->transfer_buffer);
+ dbg(4," %s : leave, status %d", __FUNCTION__, urb->status);
+}
+
+/**
+ * adu_open
+ */
+static int adu_open(struct inode *inode, struct file *file)
+{
+ struct adu_device *dev = NULL;
+ int subminor;
+ int retval = 0;
+
+ dbg(2,"%s : enter", __FUNCTION__);
+
+ subminor = MINOR(inode->i_rdev) - ADU_MINOR_BASE;
+ if ((subminor < 0) ||
+ (subminor >= MAX_DEVICES)) {
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our minor table and get our local data for this minor */
+ if (down_interruptible(&minor_table_mutex)) {
+ retval = -ERESTARTSYS;
+ goto exit;
+ }
+ dev = minor_table[subminor];
+ if (dev == NULL) {
+ up(&minor_table_mutex);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ if (down_interruptible(&dev->sem)) {
+ up(&minor_table_mutex);
+ retval = -ERESTARTSYS;
+ goto exit;
+ }
+ up(&minor_table_mutex);
+
+ /* increment our usage count for the device */
+ ++dev->open_count;
+ dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
+
+ /* check that nobody else is using the device */
+ /* NOTE: the cleanup code will decrement the count to 1 */
+ if (dev->open_count > 1) {
+ retval = -EBUSY;
+ /* unlock this device */
+ up(&dev->sem);
+ goto error;
+ }
+
+ /* save device in the file's private structure */
+ file->private_data = dev;
+
+ /* initialize in direction */
+ dev->read_buffer_length = 0;
+
+ /* fixup first read by having urb waiting for it */
+ 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,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ if (retval != 0) {
+ err("Couldn't submit interrupt_in_urb");
+ /* we ignore failure, just notify about it */
+ /*goto error;*/
+ }
+ /* end of fixup for first read */
+
+ /* unlock this device */
+ up(&dev->sem);
+ goto exit;
+
+error:
+ adu_release_internal(dev);
+
+exit:
+ dbg(2, " %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_release_internal
+ */
+static int adu_release_internal(struct adu_device *dev)
+{
+ int retval = 0;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ if (dev->udev == NULL) {
+ /* the device was unplugged before the file was released */
+ adu_delete(dev);
+ goto exit;
+ }
+
+ /* lock this device */
+ down(&dev->sem);
+
+ /* decrement our usage count for the device */
+ --dev->open_count;
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (dev->open_count <= 0) {
+ adu_abort_transfers(dev);
+ dev->open_count = 0;
+ }
+
+ /* unlock this device */
+ up(&dev->sem);
+
+exit:
+ dbg(2," %s : leave", __FUNCTION__);
+ return retval;
+}
+
+/**
+ * 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;
+ }
+
+ dev = file->private_data;
+
+ if (dev == NULL) {
+ dbg(1," %s : object is NULL", __FUNCTION__);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* lock our device */
+ down(&dev->sem);
+
+ if (dev->open_count <= 0) {
+ dbg(1," %s : device not opened", __FUNCTION__);
+ up(&dev->sem);
+ up(&minor_table_mutex);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* unlocking device */
+ up(&dev->sem);
+
+ /* do the work */
+ retval = adu_release_internal(dev);
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_read
+ */
+static ssize_t adu_read(struct file *file, __user 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 = %Zd, file=%p", __FUNCTION__, count, file);
+
+ dev = file->private_data;
+ dbg(2," %s : dev=%p", __FUNCTION__, dev);
+ /* lock this object */
+ if (down_interruptible(&dev->sem))
+ return -ERESTARTSYS;
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ 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);
+ if (i < 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+ 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);
+ /* we have a free buffer so use it */
+ should_submit = 1;
+ } 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;
+
+ 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,
+ 0);
+
+ retval = usb_submit_urb(dev->interrupt_in_urb);
+ 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) {
+ 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,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->read_urb_finished = 0;
+ usb_submit_urb(dev->interrupt_in_urb);
+ /* we ignore failure */
+ }
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+}
+
+/**
+ * adu_write
+ */
+static ssize_t adu_write(struct file *file, const __user char *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct adu_device *dev;
+ size_t bytes_written = 0;
+ size_t bytes_to_write;
+ size_t buffer_size;
+ int retval = 0;
+ int timeout = 0;
+
+ dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
+
+ dev = file->private_data;
+
+ /* lock this object */
+ down_interruptible(&dev->sem);
+
+ /* verify that the device wasn't unplugged */
+ if (dev->udev == NULL) {
+ retval = -ENODEV;
+ err("No device or device unplugged %d", retval);
+ goto exit;
+ }
+
+ /* verify that we actually have some data to write */
+ if (count == 0) {
+ dbg(1," %s : write request of 0 bytes", __FUNCTION__);
+ goto exit;
+ }
+
+
+ while (count > 0) {
+ if (dev->interrupt_out_urb->status == -EINPROGRESS) {
+ timeout = COMMAND_TIMEOUT;
+
+ while (timeout > 0) {
+ if (signal_pending(current)) {
+ dbg(1," %s : interrupted", __FUNCTION__);
+ retval = -EINTR;
+ goto exit;
+ }
+ up(&dev->sem);
+ timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ down_interruptible(&dev->sem);
+ if (timeout > 0) {
+ break;
+ }
+ dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
+ }
+
+
+ dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
+
+ if (timeout == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit;
+ }
+
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+
+ } else {
+ dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
+
+ /* write the data into interrupt_out_buffer from userspace */
+ buffer_size = dev->interrupt_out_endpoint->wMaxPacketSize;
+ bytes_to_write = count > buffer_size ? buffer_size : count;
+ dbg(4," %s : buffer_size = %Zd, count = %Zd, bytes_to_write = %Zd",
+ __FUNCTION__, buffer_size, count, bytes_to_write);
+
+ if (copy_from_user(dev->interrupt_out_buffer, buffer, bytes_to_write) != 0) {
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ /* send off the urb */
+ FILL_INT_URB(
+ dev->interrupt_out_urb,
+ dev->udev,
+ usb_sndintpipe(dev->udev, dev->interrupt_out_endpoint->bEndpointAddress),
+ dev->interrupt_out_buffer,
+ bytes_to_write,
+ adu_interrupt_out_callback,
+ dev,
+ 0);
+
+ /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_urb->actual_length = bytes_to_write;
+ retval = usb_submit_urb(dev->interrupt_out_urb);
+ if (retval < 0) {
+ err("Couldn't submit interrupt_out_urb %d", retval);
+ goto exit;
+ }
+
+ buffer += bytes_to_write;
+ count -= bytes_to_write;
+
+ bytes_written += bytes_to_write;
+ }
+ }
+
+ retval = bytes_written;
+
+exit:
+ /* unlock the device */
+ up(&dev->sem);
+
+ dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+
+ return retval;
+}
+
+/**
+ * adu_probe
+ *
+ * Called by the usb core when a new device is connected that it thinks
+ * this driver might be interested in.
+ */
+static void *adu_probe(struct usb_device *udev,
+ unsigned int ifnum, const struct usb_device_id *id)
+{
+ struct adu_device *dev = NULL;
+ struct usb_interface *interface;
+ struct usb_interface_descriptor *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ int minor;
+ int in_end_size;
+ int out_end_size;
+ int i;
+ char name[10];
+ void *retval = NULL;
+
+ dbg(2, " %s : enter", __FUNCTION__);
+
+ if (udev == NULL) {
+ info ("udev is NULL.");
+ goto exit;
+ }
+
+ if (ifnum != 0) {
+ info ("Strange interface number %d.", ifnum);
+ goto exit;
+ }
+
+ /* select a "subminor" number (part of a minor number) */
+ down(&minor_table_mutex);
+ for (minor = 0; minor < MAX_DEVICES; ++minor) {
+ if (minor_table[minor] == NULL)
+ break;
+ }
+ if (minor >= MAX_DEVICES) {
+ info ("Too many devices plugged in, can not handle this device.");
+ goto unlock_minor_exit;
+ }
+
+ /* 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 unlock_minor_exit;
+ }
+ memset (dev, 0x00, sizeof (*dev));
+
+ init_MUTEX(&dev->sem);
+ spin_lock_init(&dev->buflock);
+ down(&dev->sem);
+ dev->udev = udev;
+
+ interface = &dev->udev->actconfig->interface[0];
+ iface_desc = &interface->altsetting[0];
+
+ //dev->interface = interface;
+ dev->minor = minor;
+ 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->interrupt_out_buffer = NULL;
+ dev->interrupt_out_endpoint = NULL;
+ dev->interrupt_out_urb = NULL;
+
+ dev->read_urb_finished = 0;
+
+ /* set up the endpoint information */
+ for (i = 0; i < iface_desc->bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i];
+
+ /**
+ * Check if the endpoint has IN direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_IN) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt in endpoint */
+ dev->interrupt_in_endpoint = endpoint;
+ }
+
+ /**
+ * Check if the endpoint has OUT direction
+ * and check if the endpoint has interrupt transfer type
+ */
+ if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT)) {
+ /* we found an interrupt out endpoint */
+ dev->interrupt_out_endpoint = endpoint;
+ }
+ }
+
+ if(dev->interrupt_in_endpoint == NULL) {
+ err("interrupt in endpoint not found");
+ goto error;
+ }
+ if (dev->interrupt_out_endpoint == NULL) {
+ err("interrupt out endpoint not found");
+ goto error;
+ }
+
+ in_end_size = dev->interrupt_in_endpoint->wMaxPacketSize;
+ out_end_size = dev->interrupt_out_endpoint->wMaxPacketSize;
+
+ dev->read_buffer_primary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_primary) {
+ err("Couldn't allocate read_buffer_primary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_primary, 'a', in_end_size);
+ memset(dev->read_buffer_primary + in_end_size, 'b', in_end_size);
+ memset(dev->read_buffer_primary + (2 * in_end_size), 'c', in_end_size);
+ memset(dev->read_buffer_primary + (3 * in_end_size), 'd', in_end_size);
+
+ dev->read_buffer_secondary = kmalloc((4 * in_end_size), GFP_KERNEL);
+ if (!dev->read_buffer_secondary) {
+ err("Couldn't allocate read_buffer_secondary");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->read_buffer_secondary, 'e', in_end_size);
+ memset(dev->read_buffer_secondary + in_end_size, 'f', in_end_size);
+ memset(dev->read_buffer_secondary + (2 * in_end_size), 'g', in_end_size);
+ memset(dev->read_buffer_secondary + (3 * in_end_size), 'h', in_end_size);
+
+ dev->interrupt_in_buffer = kmalloc(in_end_size, GFP_KERNEL);
+ if (!dev->interrupt_in_buffer) {
+ err("Couldn't allocate interrupt_in_buffer");
+ goto error;
+ }
+
+ /* debug code prime the buffer */
+ memset(dev->interrupt_in_buffer, 'i', in_end_size);
+
+ dev->interrupt_in_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_in_urb) {
+ err("Couldn't allocate interrupt_in_urb");
+ goto error;
+ }
+ dev->interrupt_out_buffer = kmalloc(out_end_size, GFP_KERNEL);
+ if (!dev->interrupt_out_buffer) {
+ err("Couldn't allocate interrupt_out_buffer");
+ goto error;
+ }
+ dev->interrupt_out_urb = usb_alloc_urb(0);
+ if (!dev->interrupt_out_urb) {
+ err("Couldn't allocate interrupt_out_urb");
+ goto error;
+ }
+
+ /* put dev in interface->private_data for disconnect */
+ udev->actconfig->interface[0].private_data = dev;
+
+ /* publish it */
+ minor_table[minor] = dev;
+
+ /* initialize the devfs node for this device and register it */
+ sprintf(name, "adutux%d", dev->minor);
+
+ dev->devfs = devfs_register(usb_devfs_handle, name,
+ DEVFS_FL_DEFAULT, USB_MAJOR,
+ ADU_MINOR_BASE + dev->minor,
+ S_IFCHR | S_IRUSR | S_IWUSR |
+ S_IRGRP | S_IWGRP | S_IROTH,
+ &adu_fops, NULL);
+
+ /* let the user know what node this device is now attached to */
+ info ("ADU%d now attached to /dev/usb/adutux%d", udev->descriptor.idProduct, dev->minor);
+
+ retval = dev;
+ /* unlock device */
+ up(&dev->sem);
+ goto unlock_minor_exit;
+
+error:
+ /* unlock device */
+ up(&dev->sem);
+ adu_delete(dev);
+ dev = NULL;
+
+unlock_minor_exit:
+ up(&minor_table_mutex);
+
+exit:
+ dbg(2," %s : leave, return value 0x%.8lx (dev)", __FUNCTION__, (dev)?(long)dev:0);
+
+ return retval;
+}
+
+/**
+ * adu_disconnect
+ *
+ * Called by the usb core when the device is removed from the system.
+ */
+static void adu_disconnect(struct usb_device *udev, void *ptr)
+{
+ struct adu_device *dev;
+ int minor;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ dev = (struct adu_device *)ptr;
+
+ down (&dev->sem);
+
+ minor = dev->minor;
+
+ /* remove our devfs node */
+ devfs_unregister(dev->devfs);
+
+ /* if the device is not opened, then we clean up right now */
+ dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
+ if (!dev->open_count) {
+ up (&dev->sem);
+ adu_delete(dev);
+ } else {
+ dev->udev = NULL;
+ up (&dev->sem);
+ }
+
+ info("ADU device adutux%d now disconnected", minor);
+
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+/**
+ * adu_init
+ */
+static int __init adu_init(void)
+{
+ int result;
+
+ dbg(2," %s : enter", __FUNCTION__);
+
+ /* register this driver with the USB subsystem */
+ result = usb_register(&adu_driver);
+ if (result < 0) {
+ err("usb_register failed for the "__FILE__" driver. "
+ "Error number %d", result);
+ goto exit;
+ }
+
+ info("adutux " DRIVER_DESC " " DRIVER_VERSION);
+ info("adutux is an experimental driver. Use at your own risk");
+
+exit:
+ dbg(2," %s : leave, return value %d", __FUNCTION__, result);
+
+ return result;
+}
+
+/**
+ * adu_exit
+ */
+static void __exit adu_exit(void)
+{
+ dbg(2," %s : enter", __FUNCTION__);
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&adu_driver);
+ dbg(2," %s : leave", __FUNCTION__);
+}
+
+module_init(adu_init);
+module_exit(adu_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff -uprN -X dontdiff linux-2.4.35.3/drivers/usb/Config.in linux-2.4.35.3.build/drivers/usb/Config.in
--- linux-2.4.35.3/drivers/usb/Config.in 2007-10-16 20:52:34.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Config.in 2007-10-10 19:49:19.000000000 +0300
@@ -102,6 +102,7 @@ if [ "$CONFIG_USB" = "y" -o "$CONFIG_US

comment 'USB Miscellaneous drivers'
dep_tristate ' USB Diamond Rio500 support (EXPERIMENTAL)' CONFIG_USB_RIO500 $CONFIG_USB $CONFIG_EXPERIMENTAL
+ dep_tristate ' ADU devices from Ontrak Control Systems (EXPERIMENTAL)' CONFIG_USB_ADUTUX $CONFIG_USB $CONFIG_EXPERIMENTAL
dep_tristate ' Auerswald device support' CONFIG_USB_AUERSWALD $CONFIG_USB
dep_tristate ' Texas Instruments Graph Link USB (aka SilverLink) cable support' CONFIG_USB_TIGL $CONFIG_USB
dep_tristate ' Tieman Voyager USB Braille display support (EXPERIMENTAL)' CONFIG_USB_BRLVGER $CONFIG_USB $CONFIG_EXPERIMENTAL
diff -uprN -X dontdiff linux-2.4.35.3/drivers/usb/Makefile linux-2.4.35.3.build/drivers/usb/Makefile
--- linux-2.4.35.3/drivers/usb/Makefile 2007-10-16 20:52:34.000000000 +0300
+++ linux-2.4.35.3.build/drivers/usb/Makefile 2007-10-10 19:49:19.000000000 +0300
@@ -112,6 +112,7 @@ obj-$(CONFIG_USB_CATC) += catc.o
obj-$(CONFIG_USB_KAWETH) += kaweth.o
obj-$(CONFIG_USB_CDCETHER) += CDCEther.o
obj-$(CONFIG_USB_RIO500) += rio500.o
+obj-$(CONFIG_USB_ADUTUX) += adutux.o
obj-$(CONFIG_USB_TIGL) += tiglusb.o
obj-$(CONFIG_USB_DSBR) += dsbr100.o
obj-$(CONFIG_USB_MICROTEK) += microtek.o
diff -uprN -X dontdiff linux-2.4.35.3/MAINTAINERS linux-2.4.35.3.build/MAINTAINERS
--- linux-2.4.35.3/MAINTAINERS 2007-10-16 20:52:11.000000000 +0300
+++ linux-2.4.35.3.build/MAINTAINERS 2007-10-16 16:31:14.000000000 +0300
@@ -173,6 +173,11 @@ M: Thorsten Knabe <linux@thorsten-knabe.
W: http://linux.thorsten-knabe.de
S: Maintained

+ADUTUX ONTRAK CONTROL SYSTEMS USB DRIVER
+P: Vitaliy Ivanov
+M: [email protected]
+S: Maintained
+
ADVANSYS SCSI DRIVER
P: Bob Frey
M: [email protected]


2007-10-19 15:26:45

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On 10/17/07, Vitaliy Ivanov <[email protected]> wrote:
> Willy & Pete,
>
> On Tue, 2007-10-16 at 21:24, Vitaliy Ivanov wrote:
> > Following Pete notes I will rework code and give it for review once again.
> > As I said it's usb-skeleton approach that I reused during the port.
> >
> > Anyway, will get back to this soon.
>
> Reworked code a little.
>
> Issue with locks...
> Now minor_table_mutex is used to protect the minor_table structure and nothing else.
> It was its first purpose.
>
> dev->sem is used to protect device manipulations. It's a normal practice in 2.4.
> So I leave it this way. I think it's OK, Pete?
>
> So the final patch, hope it's final, no?:)
>
> P.S. Willy all the details you asked before can be found in my previous mail(description, device list, etc).
>

Hi all,

Didn't here anything on this? What is our final decision here?

Thanks,
Vitaliy

2007-10-19 16:53:36

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Fri, 19 Oct 2007 18:26:35 +0300, "Vitaliy Ivanov" <[email protected]> wrote:

> Didn't here anything on this? What is our final decision here?

It's gotten worse, not better. Apparently, you aren't getting the
concept of protecting the open count with a static lock and my
explanations are just not vivid enough or something. So I decided
to fix it myself. Maybe then the patch in C will explain it better
than English. But I didn't have time to do it.

Also, there's an outright bug in the latest version. Your purge
of the wrong lock was incomplete and so there was an unbalanced up().
But this is moot.

So, the version before the latest is borderline acceptable. If Willy
wants to take it, it's fine. I'll fix it up later together with 2.6.

-- Pete

2007-10-19 17:40:50

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.


> > Didn't here anything on this? What is our final decision here?
>
> It's gotten worse, not better. Apparently, you aren't getting the
> concept of protecting the open count with a static lock and my
> explanations are just not vivid enough or something. So I decided
> to fix it myself. Maybe then the patch in C will explain it better
> than English. But I didn't have time to do it.

Probably I'm not trying to do what you want. I analyzed locks for other
usb drivers in 2.4 tree and used same ideas.
Static lock minor_table_mutex is used for minor table structure.
And dev->sem for dev manipulations and that's why for open_count.
If you will simply browse /drivers/usb dir for 2.4 you will see that
such approach is widely used there.
What's not right?
Certainly, you have more experience so I can't say that I'm right.

> Also, there's an outright bug in the latest version. Your purge
> of the wrong lock was incomplete and so there was an unbalanced up().
> But this is moot.

Yes, got it. It's up for minor_table_mutex in adu_release. Corrected.

> So, the version before the latest is borderline acceptable. If Willy
> wants to take it, it's fine. I'll fix it up later together with 2.6.

Let's do everything correctly for 2.4.

V.

2007-10-23 03:45:55

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Fri, 19 Oct 2007 20:40:35 +0300, Vitaliy Ivanov <[email protected]> wrote:

Hi, Vitaly, I added you on cc: for the 2.6 cleanup. Please double-check
what I'm doing there and use it for your 2.4 version. I hope my intentions
get clearer with an example. Now, about the specific question:

> Static lock minor_table_mutex is used for minor table structure.
> And dev->sem for dev manipulations and that's why for open_count.
> If you will simply browse /drivers/usb dir for 2.4 you will see that
> such approach is widely used there.
> What's not right?

The fundamental reason why you cannot lock a free-able structure with
an in-structure lock is this. Imagine thread A locks in order to process
a disconnect. Thread B wants to open and waits for the lock. Notice that
the struct is not open, so thread A frees it. At this point, thread B
is using a freed memory.

The solution is to lock the instance struct dev with dev->mtx, except
for the open count, which is locked by a static lock (I'm ignoring
interrupts here, which cannot use mutexes).

I'm sorry to say, you're quite right: a number of drivers in 2.4 got
it wrong, and some (like adutux) carried it through 2.6.23.

-- Pete

2007-11-04 14:06:19

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Mon, Oct 22, 2007 at 08:45:39PM -0700, Pete Zaitcev wrote:
> On Fri, 19 Oct 2007 20:40:35 +0300, Vitaliy Ivanov <[email protected]> wrote:
>
> Hi, Vitaly, I added you on cc: for the 2.6 cleanup. Please double-check
> what I'm doing there and use it for your 2.4 version. I hope my intentions
> get clearer with an example. Now, about the specific question:
>
> > Static lock minor_table_mutex is used for minor table structure.
> > And dev->sem for dev manipulations and that's why for open_count.
> > If you will simply browse /drivers/usb dir for 2.4 you will see that
> > such approach is widely used there.
> > What's not right?
>
> The fundamental reason why you cannot lock a free-able structure with
> an in-structure lock is this. Imagine thread A locks in order to process
> a disconnect. Thread B wants to open and waits for the lock. Notice that
> the struct is not open, so thread A frees it. At this point, thread B
> is using a freed memory.
>
> The solution is to lock the instance struct dev with dev->mtx, except
> for the open count, which is locked by a static lock (I'm ignoring
> interrupts here, which cannot use mutexes).
>
> I'm sorry to say, you're quite right: a number of drivers in 2.4 got
> it wrong, and some (like adutux) carried it through 2.6.23.

Vitaly,

I'm planning on issuing a new 2.4.36 prerelease soon. Have you made any
progress on your code after Pete's recommendations ?

Thanks,
Willy

2007-11-05 09:32:55

by Vitaliy Ivanov

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

Willy,

On 11/4/07, Willy Tarreau <[email protected]> wrote:
> I'm planning on issuing a new 2.4.36 prerelease soon. Have you made any
> progress on your code after Pete's recommendations ?

Yep. We just finalized 2.6 changes and now I'm working on 2.4 version.
Will get back to you all soon. When are you plan to make prerelease?
As always I'm too busy on my regular work and will try to put more
time to this.

Thanks,
Vitaliy

2007-11-05 09:36:38

by Willy Tarreau

[permalink] [raw]
Subject: Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.

On Mon, Nov 05, 2007 at 11:32:46AM +0200, Vitaliy Ivanov wrote:
> Willy,
>
> On 11/4/07, Willy Tarreau <[email protected]> wrote:
> > I'm planning on issuing a new 2.4.36 prerelease soon. Have you made any
> > progress on your code after Pete's recommendations ?
>
> Yep. We just finalized 2.6 changes and now I'm working on 2.4 version.
> Will get back to you all soon. When are you plan to make prerelease?

Probably around next week-end.

> As always I'm too busy on my regular work and will try to put more
> time to this.

I can understand, of course. There's no reason to hurry. If my
prerelease is ready before your patch, I'll release it anyway. It's
just that the sooner it gets merged, the better for everyone.

Thanks,
Willy