Issue with USB/i2C

PATCH Christian ROSALIE 1 participant 1 message Feb 16, 2015

Subsystems (2)

I2C subsystem host drivers I2C

lore

Share & tools

lore

Christian ROSALIE

author

2015-02-16 16:33 UTC #0 lore raw

Hello,

I've got an issue with and USB/i2c device when i plug and unplug several times the usb device with a user-application using /dev/i2c-* file the kernel crashes because the data containing the struct i2c_adapter has been dealocated.

In the probe function of my driver, i allocates my resource (an internal structure) that contains a struct i2c_adapter with the struct i2c_algorithm. In my disconnect function i unregister the i2c adapter and then i free the ressource. Take not that i use kref design patern to prevent free data that are still used.

I look deeper in the i2c-dev (even on 3.19 release) and i notice that the callback function in the struct file_operations use struct i2c_client with an reference on a struct i2c_adapter and there is no mechanism to be sure that this reference has not be deallocated.

When i unplug my usb-device, i unregister my i2c-adapter but my user-space application still hold a file descriptor with a struc i2c_client that point to an struct i2c_adapter already freed.

I try to make a lock mechanism by adding a wait queue in the struct i2c_dev, to be sure that the adapter is no more used after a call to i2c_del_adapter, but it does not work and most of the time put the kernel in a dead lock.

To conclude it is not a good solution (not safe) to lock in i2c_del_adapter, because if the usb device is plugged again before the user-space application use the file descriptor the kernel is locked. A thread is holding core_lock mutex calling __process_removed_adapter waiting for a call on release to unlock the wait_queue while another trying to register the new adapter is locked on core_lock mutex.

I enclose my patch with this mail, knowing that it is a bad solution. Does anyone with a better knowledge on i2c-core has a working solution to this issue?

Christian ROSALIE

drivers/i2c/i2c-dev.c +48 -3

index 71c7a39..1754795 100644
@@ -47,20 +47,32 @@ struct i2c_dev {
 	struct list_head list;
 	struct i2c_adapter *adap;
 	struct device *dev;
+
+	wait_queue_head_t crelease;
+	atomic_t ccount; /* client counter */
+	atomic_t detach; /* detach is pending */
 };
 
 #define I2C_MINORS	256
 static LIST_HEAD(i2c_dev_list);
 static DEFINE_SPINLOCK(i2c_dev_list_lock);
 
-static struct i2c_dev *i2c_dev_get_by_minor(unsigned index)
+#define i2c_dev_get_by_minor(index) __i2c_dev_get_by_minor(index, 0)
+
+static struct i2c_dev *__i2c_dev_get_by_minor(unsigned index, unsigned detach)
 {
 	struct i2c_dev *i2c_dev;
 
 	spin_lock(&i2c_dev_list_lock);
 	list_for_each_entry(i2c_dev, &i2c_dev_list, list) {
-		if (i2c_dev->adap->nr == index)
+		if (i2c_dev->adap->nr == index) {
+			if (detach) {
+				/* detach is pending */
+				atomic_set(&i2c_dev->detach, 1);
+			}
+
 			goto found;
+		}
 	}
 	i2c_dev = NULL;
 found:

@@ -82,6 +94,9 @@ static struct i2c_dev *get_free_i2c_dev(struct i2c_adapter *adap)
 	if (!i2c_dev)
 		return ERR_PTR(-ENOMEM);
 	i2c_dev->adap = adap;
+	atomic_set(&i2c_dev->ccount, 0);
+	atomic_set(&i2c_dev->detach, 0);
+	init_waitqueue_head(&i2c_dev->crelease);
 
 	spin_lock(&i2c_dev_list_lock);
 	list_add_tail(&i2c_dev->list, &i2c_dev_list);

@@ -94,6 +109,7 @@ static void return_i2c_dev(struct i2c_dev *i2c_dev)
 	spin_lock(&i2c_dev_list_lock);
 	list_del(&i2c_dev->list);
 	spin_unlock(&i2c_dev_list_lock);
+
 	kfree(i2c_dev);
 }

@@ -492,6 +508,15 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	if (!i2c_dev)
 		return -ENODEV;
 
+	/* detach is pending
+	   no more client is allowed */
+	if (atomic_read(&i2c_dev->detach)) {
+		return -ENODEV;
+	}
+
+	atomic_inc(&i2c_dev->ccount);
+
+
 	adap = i2c_get_adapter(i2c_dev->adap->nr);
 	if (!adap)
 		return -ENODEV;

@@ -505,6 +530,7 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	 */
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
+		atomic_dec(&i2c_dev->ccount);
 		i2c_put_adapter(adap);
 		return -ENOMEM;
 	}

@@ -513,14 +539,26 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	client->adapter = adap;
 	file->private_data = client;
 
+
 	return 0;
 }
 
 static int i2cdev_release(struct inode *inode, struct file *file)
 {
 	struct i2c_client *client = file->private_data;
+	struct i2c_dev *i2c_dev;
 
 	i2c_put_adapter(client->adapter);
+
+	i2c_dev = i2c_dev_get_by_minor(client->adapter->nr);
+
+	if (i2c_dev) {
+		if (atomic_dec_and_test(&i2c_dev->ccount)) {
+			/* wake up wait queue if necessary */
+			wake_up_interruptible(&i2c_dev->crelease);
+		}
+	}
+
 	kfree(client);
 	file->private_data = NULL;

@@ -581,10 +619,17 @@ static int i2cdev_detach_adapter(struct device *dev, void *dummy)
 		return 0;
 	adap = to_i2c_adapter(dev);
 
-	i2c_dev = i2c_dev_get_by_minor(adap->nr);
+	i2c_dev = __i2c_dev_get_by_minor(adap->nr, 1);
 	if (!i2c_dev) /* attach_adapter must have failed */
 		return 0;
 
+	device_remove_file(i2c_dev->dev, &dev_attr_name);
+
+	/* wait that every client registered has released
+	   their reference to the adapter */
+	wait_event_interruptible(i2c_dev->crelease,
+				!atomic_read(&i2c_dev->ccount));
+
 	return_i2c_dev(i2c_dev);
 	device_destroy(i2c_dev_class, MKDEV(I2C_MAJOR, adap->nr));