[PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Hi,

Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses the 
following problems:

a) Locking problem when unplugging and then replugging USB adapter.
b) Race conditions between adding/removing devices from the devlist, while 
simultaneously loading/unloading extension modules.
c) Resource leaks on error paths.
d) Preventing the DVB framework from trying to put the adapter into sleep mode 
when the adapter has been physically unplugged. (This results in occasional 
"-19" errors from I2C functions when disconnecting.)
e) Use atomic bit operations to manage "device in use" slots, and enforce upper 
limit of EM28XX_MAXBOARDS slots.

BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?

Any review comments welcome,

Cheers,
Chris


[-- Attachment #2: EM28xx-DVB-5.diff --]
[-- Type: text/x-patch, Size: 11364 bytes --]

--- linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c.orig	2011-08-18 16:56:02.000000000 +0100
+++ linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c	2011-08-18 16:56:08.000000000 +0100
@@ -778,7 +778,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
 
-static struct dvb_frontend_ops cxd2820r_ops[2];
+static const struct dvb_frontend_ops cxd2820r_ops[2];
 
 struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
 	struct i2c_adapter *i2c, struct dvb_frontend *fe)
@@ -844,7 +844,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_attach);
 
-static struct dvb_frontend_ops cxd2820r_ops[2] = {
+static const struct dvb_frontend_ops cxd2820r_ops[2] = {
 	{
 		/* DVB-T/T2 */
 		.info = {
--- linux-3.0/drivers/media/common/tuners/tda18271-fe.c.orig	2011-08-18 16:55:53.000000000 +0100
+++ linux-3.0/drivers/media/common/tuners/tda18271-fe.c	2011-08-18 16:56:08.000000000 +0100
@@ -1230,7 +1230,7 @@
 	return 0;
 }
 
-static struct dvb_tuner_ops tda18271_tuner_ops = {
+static const struct dvb_tuner_ops tda18271_tuner_ops = {
 	.info = {
 		.name = "NXP TDA18271HD",
 		.frequency_min  =  45000000,
--- linux-3.0/drivers/media/video/em28xx/em28xx-video.c.orig	2011-08-18 17:20:10.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-video.c	2011-08-18 17:20:33.000000000 +0100
@@ -2202,6 +2202,7 @@
 		   free the remaining resources */
 		if (dev->state & DEV_DISCONNECTED) {
 			em28xx_release_resources(dev);
+			kfree(dev->alt_max_pkt_size);
 			kfree(dev);
 			return 0;
 		}
--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-17 08:52:19.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 16:56:08.000000000 +0100
@@ -60,7 +60,7 @@
 module_param_array(card,  int, NULL, 0444);
 MODULE_PARM_DESC(card,     "card type");
 
-/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS */
+/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
 
 struct em28xx_hash_table {
@@ -2738,9 +2738,9 @@
 #endif /* CONFIG_MODULES */
 
 /*
- * em28xx_realease_resources()
+ * em28xx_release_resources()
  * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
+ * called when the device gets disconnected or at module unload
 */
 void em28xx_release_resources(struct em28xx *dev)
 {
@@ -2754,8 +2754,6 @@
 
 	em28xx_release_analog_resources(dev);
 
-	em28xx_remove_from_devlist(dev);
-
 	em28xx_i2c_unregister(dev);
 
 	v4l2_device_unregister(&dev->v4l2_dev);
@@ -2763,7 +2761,7 @@
 	usb_put_dev(dev->udev);
 
 	/* Mark device as unused */
-	em28xx_devused &= ~(1 << dev->devno);
+	clear_bit(dev->devno, &em28xx_devused);
 };
 
 /*
@@ -2776,7 +2774,6 @@
 {
 	struct em28xx *dev = *devhandle;
 	int retval;
-	int errCode;
 
 	dev->udev = udev;
 	mutex_init(&dev->ctrl_urb_lock);
@@ -2872,12 +2869,11 @@
 	}
 
 	/* register i2c bus */
-	errCode = em28xx_i2c_register(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_i2c_register(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: em28xx_i2c_register - errCode [%d]!\n",
-			__func__, errCode);
-		return errCode;
+			__func__, retval);
+		goto fail_reg_i2c;
 	}
 
 	/*
@@ -2891,11 +2887,11 @@
 	em28xx_card_setup(dev);
 
 	/* Configure audio */
-	errCode = em28xx_audio_setup(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_audio_setup(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: Error while setting audio - errCode [%d]!\n",
-			__func__, errCode);
+			__func__, retval);
+		goto fail_setup_audio;
 	}
 
 	/* wake i2c devices */
@@ -2909,31 +2905,28 @@
 
 	if (dev->board.has_msp34xx) {
 		/* Send a reset to other chips via gpio */
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(1) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(2) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 	}
 
-	em28xx_add_into_devlist(dev);
-
 	retval = em28xx_register_analog_devices(dev);
 	if (retval < 0) {
-		em28xx_release_resources(dev);
-		goto fail_reg_devices;
+		goto fail_reg_analog_devices;
 	}
 
 	em28xx_init_extension(dev);
@@ -2943,7 +2936,14 @@
 
 	return 0;
 
-fail_reg_devices:
+fail_setup_audio:
+fail_write_reg:
+fail_reg_analog_devices:
+	em28xx_i2c_unregister(dev);
+
+fail_reg_i2c:
+	v4l2_device_unregister(&dev->v4l2_dev);
+
 	return retval;
 }
 
@@ -2967,8 +2967,14 @@
 	ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 
 	/* Check to see next free device and mark as used */
-	nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
-	em28xx_devused |= 1<<nr;
+	do {
+		nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
+		if (nr >= EM28XX_MAXBOARDS) {
+			/* No free device slots */
+			retval = -ENODEV;
+			goto err_no_slot;
+		}
+	} while (test_and_set_bit(nr, &em28xx_devused));
 
 	/* Don't register audio interfaces */
 	if (interface->altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
@@ -2979,7 +2985,6 @@
 			ifnum,
 			interface->altsetting[0].desc.bInterfaceClass);
 
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
@@ -3013,7 +3018,6 @@
 			em28xx_err(DRIVER_NAME " This is an anciliary "
 				"interface not used by the driver\n");
 
-			em28xx_devused &= ~(1<<nr);
 			retval = -ENODEV;
 			goto err;
 		}
@@ -3063,7 +3067,6 @@
 		printk(DRIVER_NAME ": Device initialization failed.\n");
 		printk(DRIVER_NAME ": Device must be connected to a high-speed"
 		       " USB 2.0 port.\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
@@ -3071,7 +3074,6 @@
 	if (nr >= EM28XX_MAXBOARDS) {
 		printk(DRIVER_NAME ": Supports only %i em28xx boards.\n",
 				EM28XX_MAXBOARDS);
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENOMEM;
 		goto err;
 	}
@@ -3080,12 +3082,11 @@
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (dev == NULL) {
 		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENOMEM;
 		goto err;
 	}
 
-	snprintf(dev->name, 29, "em28xx #%d", nr);
+	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
 	dev->alt   = -1;
@@ -3107,7 +3108,6 @@
 
 	if (dev->alt_max_pkt_size == NULL) {
 		em28xx_errdev("out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		kfree(dev);
 		retval = -ENOMEM;
 		goto err;
@@ -3127,7 +3127,7 @@
 	mutex_lock(&dev->lock);
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
-		em28xx_devused &= ~(1<<dev->devno);
+		kfree(dev->alt_max_pkt_size);
 		mutex_unlock(&dev->lock);
 		kfree(dev);
 		goto err;
@@ -3146,12 +3146,16 @@
 	return 0;
 
 err:
+	clear_bit(nr, &em28xx_devused);
+
+err_no_slot:
+	usb_put_dev(udev);
 	return retval;
 }
 
 /*
  * em28xx_usb_disconnect()
- * called when the device gets diconencted
+ * called when the device gets disconnected
  * video device will be unregistered on v4l2_close in case it is still open
  */
 static void em28xx_usb_disconnect(struct usb_interface *interface)
--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-17 08:52:25.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-18 16:56:08.000000000 +0100
@@ -1160,25 +1160,6 @@
 static DEFINE_MUTEX(em28xx_devlist_mutex);
 
 /*
- * em28xx_realease_resources()
- * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
-*/
-void em28xx_remove_from_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_del(&dev->devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
-void em28xx_add_into_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_add_tail(&dev->devlist, &em28xx_devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
-/*
  * Extension interface
  */
 
@@ -1193,8 +1174,8 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->init(dev);
 	}
-	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	return 0;
 }
 EXPORT_SYMBOL(em28xx_register_extension);
@@ -1207,36 +1188,34 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->fini(dev);
 	}
-	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 	list_del(&ops->next);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 }
 EXPORT_SYMBOL(em28xx_unregister_extension);
 
 void em28xx_init_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->init)
-				ops->init(dev);
-		}
+	list_add_tail(&dev->devlist, &em28xx_devlist);
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->init)
+			ops->init(dev);
 	}
 	mutex_unlock(&em28xx_devlist_mutex);
 }
 
 void em28xx_close_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->fini)
-				ops->fini(dev);
-		}
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->fini)
+			ops->fini(dev);
 	}
+	list_del(&dev->devlist);
 	mutex_unlock(&em28xx_devlist_mutex);
 }
--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-17 08:52:30.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-18 17:06:51.000000000 +0100
@@ -542,7 +542,6 @@
 	dev->dvb = dvb;
 	dvb->fe[0] = dvb->fe[1] = NULL;
 
-	mutex_lock(&dev->lock);
 	em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	/* init frontend */
 	switch (dev->model) {
@@ -711,7 +710,6 @@
 	em28xx_info("Successfully loaded em28xx-dvb\n");
 ret:
 	em28xx_set_mode(dev, EM28XX_SUSPEND);
-	mutex_unlock(&dev->lock);
 	return result;
 
 out_free:
@@ -720,6 +718,12 @@
 	goto ret;
 }
 
+static inline void prevent_sleep(struct dvb_frontend_ops *ops) {
+	ops->set_voltage = NULL;
+	ops->sleep = NULL;
+	ops->tuner_ops.sleep = NULL;
+}
+
 static int dvb_fini(struct em28xx *dev)
 {
 	if (!dev->board.has_dvb) {
@@ -728,8 +732,17 @@
 	}
 
 	if (dev->dvb) {
-		unregister_dvb(dev->dvb);
-		kfree(dev->dvb);
+		struct em28xx_dvb *dvb = dev->dvb;
+
+		if (dev->state & DEV_DISCONNECTED) {
+			/* We cannot tell the device to sleep
+			 * once it has been unplugged. */
+			prevent_sleep(&dvb->fe[0]->ops);
+			prevent_sleep(&dvb->fe[1]->ops);
+		}
+
+		unregister_dvb(dvb);
+		kfree(dvb);
 		dev->dvb = NULL;
 	}
 

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Devin Heitmueller]

On Thu, Aug 18, 2011 at 1:52 PM, Chris Rankin <rankincj@yahoo.com> wrote:
> Hi,
>
> Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses
> the following problems:
>
> a) Locking problem when unplugging and then replugging USB adapter.
> b) Race conditions between adding/removing devices from the devlist, while
> simultaneously loading/unloading extension modules.
> c) Resource leaks on error paths.
> d) Preventing the DVB framework from trying to put the adapter into sleep
> mode when the adapter has been physically unplugged. (This results in
> occasional "-19" errors from I2C functions when disconnecting.)
> e) Use atomic bit operations to manage "device in use" slots, and enforce
> upper limit of EM28XX_MAXBOARDS slots.

Hi Chris,

You would be well served to break this into a patch series, as it
tends to be difficult to review a whole series of changes in a single
patch.

You seem to be mixed in a bunch of "useless" changes alongside
functional changes.  For example, if you're trying to add in a missing
goto inside an exception block, doing that at the same time as
renaming instances of "errCode" to "retval" just creates confusion.

And finally, the mutex structure used for the modules is somewhat
complicated due to to the need to keep the analog side of the board
locked while initializing the digital side.  This code was added
specifically to prevent race conditions that were seen during
initialization as things like udev and dbus attempted to connect to
the newly created V4L device while the em28xx-dvb module was still
coming up.

In other words, I don't doubt there are bugs, and I cannot say whether
your fixes are appropriate without giving a hard look at the logic.
But you should be aware of the thinking behind the way it was done and
it would be very worthwhile if you could test with at least one hybrid
product to ensure the changes you are making don't break anything (the
em2874 used in the 290e is a poor test case since it doesn't have
analog support).

> BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?

This is largely a product of the history of the devices using the
framework.  The em28xx driver was originally analog only, and DVB
support was added later as new chips came out which needed it.  The
dvb-usb driver came from dedicated DVB devices that had no analog
support.  In fact, even today the lack of analog support is a huge
deficiency in that framework which is why we don't support the analog
side of any hybrid devices that use dvb-usb.

In other words, if we were reinventing this stuff today, there would
probably be only a single framework shared by dvb-usb and em28xx.  But
at this point it's too much cost and too little benefit to go through
the work to attempt to merge them.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Devin Heitmueller]

On Thu, Aug 18, 2011 at 2:43 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Thu, Aug 18, 2011 at 1:52 PM, Chris Rankin <rankincj@yahoo.com> wrote:
>> Hi,
>>
>> Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses
>> the following problems:
>>
>> a) Locking problem when unplugging and then replugging USB adapter.
>> b) Race conditions between adding/removing devices from the devlist, while
>> simultaneously loading/unloading extension modules.
>> c) Resource leaks on error paths.
>> d) Preventing the DVB framework from trying to put the adapter into sleep
>> mode when the adapter has been physically unplugged. (This results in
>> occasional "-19" errors from I2C functions when disconnecting.)
>> e) Use atomic bit operations to manage "device in use" slots, and enforce
>> upper limit of EM28XX_MAXBOARDS slots.
>
> Hi Chris,
>
> You would be well served to break this into a patch series, as it
> tends to be difficult to review a whole series of changes in a single
> patch.
>
> You seem to be mixed in a bunch of "useless" changes alongside
> functional changes.  For example, if you're trying to add in a missing
> goto inside an exception block, doing that at the same time as
> renaming instances of "errCode" to "retval" just creates confusion.
>
> And finally, the mutex structure used for the modules is somewhat
> complicated due to to the need to keep the analog side of the board
> locked while initializing the digital side.  This code was added
> specifically to prevent race conditions that were seen during
> initialization as things like udev and dbus attempted to connect to
> the newly created V4L device while the em28xx-dvb module was still
> coming up.
>
> In other words, I don't doubt there are bugs, and I cannot say whether
> your fixes are appropriate without giving a hard look at the logic.
> But you should be aware of the thinking behind the way it was done and
> it would be very worthwhile if you could test with at least one hybrid
> product to ensure the changes you are making don't break anything (the
> em2874 used in the 290e is a poor test case since it doesn't have
> analog support).
>
>> BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?
>
> This is largely a product of the history of the devices using the
> framework.  The em28xx driver was originally analog only, and DVB
> support was added later as new chips came out which needed it.  The
> dvb-usb driver came from dedicated DVB devices that had no analog
> support.  In fact, even today the lack of analog support is a huge
> deficiency in that framework which is why we don't support the analog
> side of any hybrid devices that use dvb-usb.
>
> In other words, if we were reinventing this stuff today, there would
> probably be only a single framework shared by dvb-usb and em28xx.  But
> at this point it's too much cost and too little benefit to go through
> the work to attempt to merge them.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com
>

Probably one more point worth making:  I definitely appreciate that
you've take the time to focus on these particular problems.  I've been
complaining about them for months but just never got around to rolling
up my sleeves to debug them myself.

In other words, don't interpret anything in my previous email as discouragement.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

On 18/08/11 19:43, Devin Heitmueller wrote:
> You would be well served to break this into a patch series, as it tends to be 
> difficult to review a whole series of changes in a single patch. You seem to 
> be mixed in a bunch of "useless" changes alongside functional changes. For 
> example, if you're trying to add in a missing goto inside an exception block, 
> doing that at the same time as renaming instances of "errCode" to "retval" 
> just creates confusion.

Actually, those two particular changes go together because I'm replacing "return 
errCode" with a goto to "return retval". Ultimately, errCode becomes an unused 
variable.

> And finally, the mutex structure used for the modules is somewhat complicated 
> due to to the need to keep the analog side of the board locked while 
> initializing the digital side. This code was added specifically to prevent 
> race conditions that were seen during initialization as things like udev and 
> dbus attempted to connect to the newly created V4L device while the em28xx-dvb 
> module was still coming up.

OK, thanks. I've been tackling this problem from the "We must always take lock A 
before lock B, and never vice versa" point of view. So the order is:

- take device mutex
- enter em28xx_init_dev()
- enter em28xx_init_extension()
- take device list mutex
- call init() function for every extension with this device

Since dvb_init() is the init() function for the em28xx-dvb extension, it follows 
that it cannot take the device's mutex again. The problem is therefore moved to 
em28xx_dvb_register(), which takes the device list mutex and yet MUST not take 
the mutex for any device in the list.

Combining em28xx_add_into_devlist() with em28xx_init_extension() (and similarly 
em28xx_remove_from_devlist() with em28xx_close_extension()) means that the 
device list must always contain a list of devices that has been initialised 
against every extension in the extension list.

I can probably factor out the simpler patches first, such as using the bit 
operations on em28xx_devused, and the memory leak in em28xx_v4l2_close(). And 
the spelling fixes...

Cheers,
Chris

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On 18/08/11 19:44, Devin Heitmueller wrote:
> You would be well served to break this into a patch series, as it tends to be 
> difficult to review a whole series of changes in a single patch.

Here are the first three patches:

- release the alt_max_pkt_size buffer.
- use atomic bit operations for em28xx_devused.
- use the correct amount of memory for snprintf().

Cheers,
Chris

Signed-of-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-devused-bits.diff --]
[-- Type: text/x-patch, Size: 2929 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-18 22:24:12.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:22:14.000000000 +0100
@@ -60,7 +60,7 @@
 module_param_array(card,  int, NULL, 0444);
 MODULE_PARM_DESC(card,     "card type");
 
-/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS */
+/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
 
 struct em28xx_hash_table {
@@ -2763,7 +2763,7 @@
 	usb_put_dev(dev->udev);
 
 	/* Mark device as unused */
-	em28xx_devused &= ~(1 << dev->devno);
+	clear_bit(dev->devno, &em28xx_devused);
 };
 
 /*
@@ -2967,8 +2967,16 @@
 	ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 
 	/* Check to see next free device and mark as used */
-	nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
-	em28xx_devused |= 1<<nr;
+	do {
+		nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
+		if (nr >= EM28XX_MAXBOARDS) {
+			/* No free device slots */
+			printk(DRIVER_NAME ": Supports only %i em28xx boards.\n",
+					EM28XX_MAXBOARDS);
+			retval = -ENOMEM;
+			goto err_no_slot;
+		}
+	} while (test_and_set_bit(nr, &em28xx_devused));
 
 	/* Don't register audio interfaces */
 	if (interface->altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
@@ -2979,7 +2987,6 @@
 			ifnum,
 			interface->altsetting[0].desc.bInterfaceClass);
 
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
@@ -3013,7 +3020,6 @@
 			em28xx_err(DRIVER_NAME " This is an anciliary "
 				"interface not used by the driver\n");
 
-			em28xx_devused &= ~(1<<nr);
 			retval = -ENODEV;
 			goto err;
 		}
@@ -3063,24 +3069,14 @@
 		printk(DRIVER_NAME ": Device initialization failed.\n");
 		printk(DRIVER_NAME ": Device must be connected to a high-speed"
 		       " USB 2.0 port.\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
 
-	if (nr >= EM28XX_MAXBOARDS) {
-		printk(DRIVER_NAME ": Supports only %i em28xx boards.\n",
-				EM28XX_MAXBOARDS);
-		em28xx_devused &= ~(1<<nr);
-		retval = -ENOMEM;
-		goto err;
-	}
-
 	/* allocate memory for our device state and initialize it */
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (dev == NULL) {
 		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENOMEM;
 		goto err;
 	}
@@ -3107,7 +3103,6 @@
 
 	if (dev->alt_max_pkt_size == NULL) {
 		em28xx_errdev("out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		kfree(dev);
 		retval = -ENOMEM;
 		goto err;
@@ -3127,7 +3122,6 @@
 	mutex_lock(&dev->lock);
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
-		em28xx_devused &= ~(1<<dev->devno);
 		kfree(dev->alt_max_pkt_size);
 		mutex_unlock(&dev->lock);
 		kfree(dev);
@@ -3147,6 +3141,10 @@
 	return 0;
 
 err:
+	clear_bit(nr, &em28xx_devused);
+
+err_no_slot:
+	usb_put_dev(udev);
 	return retval;
 }
 

[-- Attachment #3: EM28xx-snprintf.diff --]
[-- Type: text/x-patch, Size: 405 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-17 08:52:19.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:03:05.000000000 +0100
@@ -3085,7 +3085,7 @@
 		goto err;
 	}
 
-	snprintf(dev->name, 29, "em28xx #%d", nr);
+	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
 	dev->alt   = -1;

[-- Attachment #4: EM28xx-video-leak.diff --]
[-- Type: text/x-patch, Size: 804 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-video.c.orig	2011-08-18 17:20:10.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-video.c	2011-08-18 17:20:33.000000000 +0100
@@ -2202,6 +2202,7 @@
 		   free the remaining resources */
 		if (dev->state & DEV_DISCONNECTED) {
 			em28xx_release_resources(dev);
+			kfree(dev->alt_max_pkt_size);
 			kfree(dev);
 			return 0;
 		}
--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-17 08:52:19.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:09:32.000000000 +0100
@@ -3128,6 +3128,7 @@
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
 		em28xx_devused &= ~(1<<dev->devno);
+		kfree(dev->alt_max_pkt_size);
 		mutex_unlock(&dev->lock);
 		kfree(dev);
 		goto err;

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

Two more patches:

a) clean up resources more reliably if em28xx_init_dev() fails,
b) move two printk() statements outside the mutex lock

Cheers,
Chris

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-init-resources.diff --]
[-- Type: text/x-patch, Size: 3872 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-18 22:42:03.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:55:52.000000000 +0100
@@ -2776,7 +2776,6 @@
 {
 	struct em28xx *dev = *devhandle;
 	int retval;
-	int errCode;
 
 	dev->udev = udev;
 	mutex_init(&dev->ctrl_urb_lock);
@@ -2858,7 +2857,7 @@
 		/* Resets I2C speed */
 		em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
 		if (retval < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req failed!"
+			em28xx_errdev("%s: em28xx_write_reg failed!"
 				      " retval [%d]\n",
 				      __func__, retval);
 			return retval;
@@ -2872,12 +2871,11 @@
 	}
 
 	/* register i2c bus */
-	errCode = em28xx_i2c_register(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_i2c_register(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: em28xx_i2c_register - errCode [%d]!\n",
-			__func__, errCode);
-		return errCode;
+			__func__, retval);
+		goto fail_reg_i2c;
 	}
 
 	/*
@@ -2891,11 +2889,11 @@
 	em28xx_card_setup(dev);
 
 	/* Configure audio */
-	errCode = em28xx_audio_setup(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_audio_setup(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: Error while setting audio - errCode [%d]!\n",
-			__func__, errCode);
+			__func__, retval);
+		goto fail_setup_audio;
 	}
 
 	/* wake i2c devices */
@@ -2909,31 +2907,28 @@
 
 	if (dev->board.has_msp34xx) {
 		/* Send a reset to other chips via gpio */
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(1) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(2) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 	}
 
-	em28xx_add_into_devlist(dev);
-
 	retval = em28xx_register_analog_devices(dev);
 	if (retval < 0) {
-		em28xx_release_resources(dev);
-		goto fail_reg_devices;
+		goto fail_reg_analog_devices;
 	}
 
 	em28xx_init_extension(dev);
@@ -2943,7 +2938,14 @@
 
 	return 0;
 
-fail_reg_devices:
+fail_setup_audio:
+fail_write_reg:
+fail_reg_analog_devices:
+	em28xx_i2c_unregister(dev);
+
+fail_reg_i2c:
+	v4l2_device_unregister(&dev->v4l2_dev);
+
 	return retval;
 }
 
--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-17 08:52:25.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-18 22:51:59.000000000 +0100
@@ -1171,13 +1171,6 @@
 	mutex_unlock(&em28xx_devlist_mutex);
 };
 
-void em28xx_add_into_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_add_tail(&dev->devlist, &em28xx_devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
 /*
  * Extension interface
  */
@@ -1215,14 +1208,13 @@
 
 void em28xx_init_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->init)
-				ops->init(dev);
-		}
+	list_add_tail(&dev->devlist, &em28xx_devlist);
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->init)
+			ops->init(dev);
 	}
 	mutex_unlock(&em28xx_devlist_mutex);
 }

[-- Attachment #3: EM28xx-unlocked-printk.diff --]
[-- Type: text/x-patch, Size: 873 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-18 23:05:50.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-18 23:07:02.000000000 +0100
@@ -1186,8 +1186,8 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->init(dev);
 	}
-	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	return 0;
 }
 EXPORT_SYMBOL(em28xx_register_extension);
@@ -1200,9 +1200,9 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->fini(dev);
 	}
-	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 	list_del(&ops->next);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 }
 EXPORT_SYMBOL(em28xx_unregister_extension);
 

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

Next patch: By default, the DVB framework tries to put a frontend to sleep after 
it has been shut down. This obviously doesn't work for a USB device that has 
been disconnected, and can result in occasional errors in dmesg about I2C writes 
failing with error code -19.

The patch works by nulling out the function pointers that the DVB framework 
would otherwise try to call. I have therefore declared the structs in the 
tda18271 and cxd2820r modules to be "const", so that we know that they are 
supposed only to be templates.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-disconnect-dont-sleep.diff --]
[-- Type: text/x-patch, Size: 1957 bytes --]

--- linux-3.0/drivers/media/common/tuners/tda18271-fe.c.orig	2011-08-18 16:55:53.000000000 +0100
+++ linux-3.0/drivers/media/common/tuners/tda18271-fe.c	2011-08-18 23:12:55.000000000 +0100
@@ -1230,7 +1230,7 @@
 	return 0;
 }
 
-static struct dvb_tuner_ops tda18271_tuner_ops = {
+static const struct dvb_tuner_ops tda18271_tuner_ops = {
 	.info = {
 		.name = "NXP TDA18271HD",
 		.frequency_min  =  45000000,
--- linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c.orig	2011-08-18 16:56:02.000000000 +0100
+++ linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c	2011-08-18 23:14:06.000000000 +0100
@@ -778,7 +778,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
 
-static struct dvb_frontend_ops cxd2820r_ops[2];
+static const struct dvb_frontend_ops cxd2820r_ops[2];
 
 struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
 	struct i2c_adapter *i2c, struct dvb_frontend *fe)
@@ -844,7 +844,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_attach);
 
-static struct dvb_frontend_ops cxd2820r_ops[2] = {
+static const struct dvb_frontend_ops cxd2820r_ops[2] = {
 	{
 		/* DVB-T/T2 */
 		.info = {
--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-17 08:52:30.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-18 23:17:42.000000000 +0100
@@ -720,6 +720,12 @@
 	goto ret;
 }
 
+static inline void prevent_sleep(struct dvb_frontend_ops *ops) {
+	ops->set_voltage = NULL;
+	ops->sleep = NULL;
+	ops->tuner_ops.sleep = NULL;
+}
+
 static int dvb_fini(struct em28xx *dev)
 {
 	if (!dev->board.has_dvb) {
@@ -728,8 +734,17 @@
 	}
 
 	if (dev->dvb) {
-		unregister_dvb(dev->dvb);
-		kfree(dev->dvb);
+		struct em28xx_dvb *dvb = dev->dvb;
+
+		if (dev->state & DEV_DISCONNECTED) {
+			/* We cannot tell the device to sleep
+			 * once it has been unplugged. */
+			prevent_sleep(&dvb->fe[0]->ops);
+			prevent_sleep(&dvb->fe[1]->ops);
+		}
+
+		unregister_dvb(dvb);
+		kfree(dvb);
 		dev->dvb = NULL;
 	}
 

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

This patch closes the race on the device and extension lists at USB disconnect 
time. Previously, the device was removed from the device list during 
em28xx_release_resources(), and then passed to the em28xx_close_extension() 
function so that all extensions could run their fini() operations. However, this 
left a (brief, theoretical, highly unlikely ;-)) window between these two calls 
during which a new module could call em28xx_register_extension(). The result 
would have been that the em28xx_usb_disconnect() function would also have passed 
the device to the new extension's fini() function, despite never having called 
the extension's init() function.

This patch also restores em28xx_close_extension()'s symmetry with 
em28xx_init_extension(), and establishes the property that every device in the 
device list must have been initialised for every extension in the extension list.

Signed-of-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-race-on-disconnect.diff --]
[-- Type: text/x-patch, Size: 2136 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-19 00:23:17.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-19 00:32:40.000000000 +0100
@@ -2738,9 +2738,9 @@
 #endif /* CONFIG_MODULES */
 
 /*
- * em28xx_realease_resources()
+ * em28xx_release_resources()
  * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
+ * called when the device gets disconnected or at module unload
 */
 void em28xx_release_resources(struct em28xx *dev)
 {
@@ -2754,8 +2754,6 @@
 
 	em28xx_release_analog_resources(dev);
 
-	em28xx_remove_from_devlist(dev);
-
 	em28xx_i2c_unregister(dev);
 
 	v4l2_device_unregister(&dev->v4l2_dev);
@@ -3152,7 +3150,7 @@
 
 /*
  * em28xx_usb_disconnect()
- * called when the device gets diconencted
+ * called when the device gets disconnected
  * video device will be unregistered on v4l2_close in case it is still open
  */
 static void em28xx_usb_disconnect(struct usb_interface *interface)
--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-18 23:07:51.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-19 00:27:00.000000000 +0100
@@ -1160,18 +1160,6 @@
 static DEFINE_MUTEX(em28xx_devlist_mutex);
 
 /*
- * em28xx_realease_resources()
- * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
-*/
-void em28xx_remove_from_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_del(&dev->devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
-/*
  * Extension interface
  */
 
@@ -1221,14 +1209,13 @@
 
 void em28xx_close_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->fini)
-				ops->fini(dev);
-		}
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->fini)
+			ops->fini(dev);
 	}
+	list_del(&dev->devlist);
 	mutex_unlock(&em28xx_devlist_mutex);
 }

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

The final patch removes the unplug/replug deadlock by not holding the device 
mutex during dvb_init(). However, this mutex has already been locked during 
device initialisation by em28xx_usb_probe() and is not released again until all 
extensions have been initialised successfully.

The device mutex is not held during either em28xx_register_extension() or 
em28xx_unregister_extension() any more. More importantly, I don't believe it can 
safely be held by these functions because they must both - by their nature -  
acquire the device list mutex before they can iterate through the device list. 
In other words, while usb_probe() and usb_disconnect() acquire the device mutex 
followed by the device list mutex, the register/unregister_extension() functions 
would need to acquire these mutexes in the opposite order. And that sounds like 
a potential deadlock.

On the other hand, the new situation is a definite improvement :-).

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-replug-deadlock.diff --]
[-- Type: text/x-patch, Size: 551 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-19 00:50:41.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-19 00:51:03.000000000 +0100
@@ -542,7 +542,6 @@
 	dev->dvb = dvb;
 	dvb->fe[0] = dvb->fe[1] = NULL;
 
-	mutex_lock(&dev->lock);
 	em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	/* init frontend */
 	switch (dev->model) {
@@ -711,7 +710,6 @@
 	em28xx_info("Successfully loaded em28xx-dvb\n");
 ret:
 	em28xx_set_mode(dev, EM28XX_SUSPEND);
-	mutex_unlock(&dev->lock);
 	return result;
 
 out_free:

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Mauro Carvalho Chehab]

Em 18-08-2011 12:56, Chris Rankin escreveu:
> On 18/08/11 19:43, Devin Heitmueller wrote:
>> You would be well served to break this into a patch series, as it tends to be difficult to review a whole series of changes in a single patch. You seem to be mixed in a bunch of "useless" changes alongside functional changes. For example, if you're trying to add in a missing goto inside an exception block, doing that at the same time as renaming instances of "errCode" to "retval" just creates confusion.
> 
> Actually, those two particular changes go together because I'm replacing "return errCode" with a goto to "return retval". Ultimately, errCode becomes an unused variable.
> 
>> And finally, the mutex structure used for the modules is somewhat complicated due to to the need to keep the analog side of the board locked while initializing the digital side. This code was added specifically to prevent race conditions that were seen during initialization as things like udev and dbus attempted to connect to the newly created V4L device while the em28xx-dvb module was still coming up.
> 
> OK, thanks. I've been tackling this problem from the "We must always take lock A before lock B, and never vice versa" point of view. So the order is:
> 
> - take device mutex
> - enter em28xx_init_dev()
> - enter em28xx_init_extension()
> - take device list mutex
> - call init() function for every extension with this device
> 
> Since dvb_init() is the init() function for the em28xx-dvb extension, it follows that it cannot take the device's mutex again. The problem is therefore moved to em28xx_dvb_register(), which takes the device list mutex and yet MUST not take the mutex for any device in the list.
> 
> Combining em28xx_add_into_devlist() with em28xx_init_extension() (and similarly em28xx_remove_from_devlist() with em28xx_close_extension()) means that the device list must always contain a list of devices that has been initialised against every extension in the extension list.
> 
> I can probably factor out the simpler patches first, such as using the bit operations on em28xx_devused, and the memory leak in em28xx_v4l2_close(). And the spelling fixes...

I agree with Devin: please break it into one patch with just the mutex
fixes, and another one(s) with the cleanups. The basic rule is one patch
per logic change. 

For the mutex patch, please add:
"Cc: stable@kernel.org" at the line before your signed-off-by, as such
patch should be backported to stable kernels.

With respect to return errCode, maybe the better would be to
rename it as just "rc". It is very common to call the return
code as that on several places, and it is smaller.

PS.: I didn't make a full review of your patches yet. I'll probably
do it by the next week, after returning from LinuxCon.

Thanks!
Mauro

Re: [PATCH] Latest version of em28xx / em28xx-dvb patch for PCTV 290e

[Mauro Carvalho Chehab]

Em 18-08-2011 15:11, Chris Rankin escreveu:
> Two more patches:
> 
> a) clean up resources more reliably if em28xx_init_dev() fails,
> b) move two printk() statements outside the mutex lock
> 
> Cheers,
> Chris
> 
> Signed-off-by: Chris Rankin <rankincj@yahoo.com>
> 
Chris,

Please, never send two patches at the same email. This doesn't
work: patchwork and all tools used by me and other maintainers
will either discard one or both patches, or even merge both into
one.

Also, when you're sending us a series of patches, be sure to
number the patch series with PATCH 01/xx (where xx is the last
patch), and put a different subject on each patch. There's no
need to reply the original thread.

See how all your patches will appear for me:

https://patchwork.kernel.org/project/linux-media/list/?submitter=4542

It is very hard to know that you have a complese series of patches,
instead of just a new version of a patch that you've already sent. 
By default, the last case is assumed, as it is somewhat common to reply
to a patch with a new version of it (yet, the best practice would be
to rename the new patch series as PATCH v2, PATCH v3, etc). If I wouldn't
notice this before I would simply mark all patches except by the last
one as superseded, and just get the last one.

Thanks!
Mauro


PS.: I suggest you to take a look at the wiki page bellow. It helps 
people about what's the expected way for you to submit your work
http://linuxtv.org/wiki/index.php/Development:_How_to_submit_patches

[PATCH 1/6 ] EM28xx - pass correct buffer size to snprintf

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 130 bytes --]

snprintf()'s size parameter includes space for the terminating '\0' character.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-snprintf.diff --]
[-- Type: text/x-patch, Size: 405 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-17 08:52:19.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:03:05.000000000 +0100
@@ -3085,7 +3085,7 @@
 		goto err;
 	}
 
-	snprintf(dev->name, 29, "em28xx #%d", nr);
+	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
 	dev->alt   = -1;

[PATCH 2/6] Fix memory leak on disconnect or error.

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Release the dev->alt_max_pkt_size buffer in all cases.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-video-leak.diff --]
[-- Type: text/x-patch, Size: 804 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-video.c.orig	2011-08-18 17:20:10.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-video.c	2011-08-18 17:20:33.000000000 +0100
@@ -2202,6 +2202,7 @@
 		   free the remaining resources */
 		if (dev->state & DEV_DISCONNECTED) {
 			em28xx_release_resources(dev);
+			kfree(dev->alt_max_pkt_size);
 			kfree(dev);
 			return 0;
 		}
--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-17 08:52:19.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:09:32.000000000 +0100
@@ -3128,6 +3128,7 @@
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
 		em28xx_devused &= ~(1<<dev->devno);
+		kfree(dev->alt_max_pkt_size);
 		mutex_unlock(&dev->lock);
 		kfree(dev);
 		goto err;

[PATCH 3/6] EM28xx - use atomic bit operations for devices-in-use mask

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

Use atomic bit operations for the em28xx_devused mask, to prevent an unlikely 
race condition should two adapters be plugged in simultaneously. The operations 
also clearer than explicit bit manipulation anyway.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-devused-bits.diff --]
[-- Type: text/x-patch, Size: 2929 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-18 22:24:12.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:22:14.000000000 +0100
@@ -60,7 +60,7 @@
 module_param_array(card,  int, NULL, 0444);
 MODULE_PARM_DESC(card,     "card type");
 
-/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS */
+/* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
 
 struct em28xx_hash_table {
@@ -2763,7 +2763,7 @@
 	usb_put_dev(dev->udev);
 
 	/* Mark device as unused */
-	em28xx_devused &= ~(1 << dev->devno);
+	clear_bit(dev->devno, &em28xx_devused);
 };
 
 /*
@@ -2967,8 +2967,16 @@
 	ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 
 	/* Check to see next free device and mark as used */
-	nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
-	em28xx_devused |= 1<<nr;
+	do {
+		nr = find_first_zero_bit(&em28xx_devused, EM28XX_MAXBOARDS);
+		if (nr >= EM28XX_MAXBOARDS) {
+			/* No free device slots */
+			printk(DRIVER_NAME ": Supports only %i em28xx boards.\n",
+					EM28XX_MAXBOARDS);
+			retval = -ENOMEM;
+			goto err_no_slot;
+		}
+	} while (test_and_set_bit(nr, &em28xx_devused));
 
 	/* Don't register audio interfaces */
 	if (interface->altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
@@ -2979,7 +2987,6 @@
 			ifnum,
 			interface->altsetting[0].desc.bInterfaceClass);
 
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
@@ -3013,7 +3020,6 @@
 			em28xx_err(DRIVER_NAME " This is an anciliary "
 				"interface not used by the driver\n");
 
-			em28xx_devused &= ~(1<<nr);
 			retval = -ENODEV;
 			goto err;
 		}
@@ -3063,24 +3069,14 @@
 		printk(DRIVER_NAME ": Device initialization failed.\n");
 		printk(DRIVER_NAME ": Device must be connected to a high-speed"
 		       " USB 2.0 port.\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENODEV;
 		goto err;
 	}
 
-	if (nr >= EM28XX_MAXBOARDS) {
-		printk(DRIVER_NAME ": Supports only %i em28xx boards.\n",
-				EM28XX_MAXBOARDS);
-		em28xx_devused &= ~(1<<nr);
-		retval = -ENOMEM;
-		goto err;
-	}
-
 	/* allocate memory for our device state and initialize it */
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (dev == NULL) {
 		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		retval = -ENOMEM;
 		goto err;
 	}
@@ -3107,7 +3103,6 @@
 
 	if (dev->alt_max_pkt_size == NULL) {
 		em28xx_errdev("out of memory!\n");
-		em28xx_devused &= ~(1<<nr);
 		kfree(dev);
 		retval = -ENOMEM;
 		goto err;
@@ -3127,7 +3122,6 @@
 	mutex_lock(&dev->lock);
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
-		em28xx_devused &= ~(1<<dev->devno);
 		kfree(dev->alt_max_pkt_size);
 		mutex_unlock(&dev->lock);
 		kfree(dev);
@@ -3147,6 +3141,10 @@
 	return 0;
 
 err:
+	clear_bit(nr, &em28xx_devused);
+
+err_no_slot:
+	usb_put_dev(udev);
 	return retval;
 }
 

[PATCH 4/6] EM28xx - clean up resources should init fail

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

This patch ensures that the em28xx_init_dev() function cleans up after itself, 
in the event that it fails. This isimportant because the struct em28xx will be 
deallocated if em28xx_init_dev() returns an error.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-init-resources.diff --]
[-- Type: text/x-patch, Size: 3872 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-18 22:42:03.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-18 22:55:52.000000000 +0100
@@ -2776,7 +2776,6 @@
 {
 	struct em28xx *dev = *devhandle;
 	int retval;
-	int errCode;
 
 	dev->udev = udev;
 	mutex_init(&dev->ctrl_urb_lock);
@@ -2858,7 +2857,7 @@
 		/* Resets I2C speed */
 		em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
 		if (retval < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req failed!"
+			em28xx_errdev("%s: em28xx_write_reg failed!"
 				      " retval [%d]\n",
 				      __func__, retval);
 			return retval;
@@ -2872,12 +2871,11 @@
 	}
 
 	/* register i2c bus */
-	errCode = em28xx_i2c_register(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_i2c_register(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: em28xx_i2c_register - errCode [%d]!\n",
-			__func__, errCode);
-		return errCode;
+			__func__, retval);
+		goto fail_reg_i2c;
 	}
 
 	/*
@@ -2891,11 +2889,11 @@
 	em28xx_card_setup(dev);
 
 	/* Configure audio */
-	errCode = em28xx_audio_setup(dev);
-	if (errCode < 0) {
-		v4l2_device_unregister(&dev->v4l2_dev);
+	retval = em28xx_audio_setup(dev);
+	if (retval < 0) {
 		em28xx_errdev("%s: Error while setting audio - errCode [%d]!\n",
-			__func__, errCode);
+			__func__, retval);
+		goto fail_setup_audio;
 	}
 
 	/* wake i2c devices */
@@ -2909,31 +2907,28 @@
 
 	if (dev->board.has_msp34xx) {
 		/* Send a reset to other chips via gpio */
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xf7);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(1) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 
-		errCode = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
-		if (errCode < 0) {
-			em28xx_errdev("%s: em28xx_write_regs_req - "
+		retval = em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xff);
+		if (retval < 0) {
+			em28xx_errdev("%s: em28xx_write_reg - "
 				      "msp34xx(2) failed! errCode [%d]\n",
-				      __func__, errCode);
-			return errCode;
+				      __func__, retval);
+			goto fail_write_reg;
 		}
 		msleep(3);
 	}
 
-	em28xx_add_into_devlist(dev);
-
 	retval = em28xx_register_analog_devices(dev);
 	if (retval < 0) {
-		em28xx_release_resources(dev);
-		goto fail_reg_devices;
+		goto fail_reg_analog_devices;
 	}
 
 	em28xx_init_extension(dev);
@@ -2943,7 +2938,14 @@
 
 	return 0;
 
-fail_reg_devices:
+fail_setup_audio:
+fail_write_reg:
+fail_reg_analog_devices:
+	em28xx_i2c_unregister(dev);
+
+fail_reg_i2c:
+	v4l2_device_unregister(&dev->v4l2_dev);
+
 	return retval;
 }
 
--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-17 08:52:25.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-18 22:51:59.000000000 +0100
@@ -1171,13 +1171,6 @@
 	mutex_unlock(&em28xx_devlist_mutex);
 };
 
-void em28xx_add_into_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_add_tail(&dev->devlist, &em28xx_devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
 /*
  * Extension interface
  */
@@ -1215,14 +1208,13 @@
 
 void em28xx_init_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->init)
-				ops->init(dev);
-		}
+	list_add_tail(&dev->devlist, &em28xx_devlist);
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->init)
+			ops->init(dev);
 	}
 	mutex_unlock(&em28xx_devlist_mutex);
 }

[PATCH 5/6] EM28xx - move printk lines outside mutex lock

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

There's no reason to still be holding the device list mutex for either of these 
printk statements.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-unlocked-printk.diff --]
[-- Type: text/x-patch, Size: 873 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-18 23:05:50.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-18 23:07:02.000000000 +0100
@@ -1186,8 +1186,8 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->init(dev);
 	}
-	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
 	return 0;
 }
 EXPORT_SYMBOL(em28xx_register_extension);
@@ -1200,9 +1200,9 @@
 	list_for_each_entry(dev, &em28xx_devlist, devlist) {
 		ops->fini(dev);
 	}
-	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 	list_del(&ops->next);
 	mutex_unlock(&em28xx_devlist_mutex);
+	printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
 }
 EXPORT_SYMBOL(em28xx_unregister_extension);
 

[PATCH 6/6] EM28xx - don't sleep on disconnect

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

The DVB framework will try to power-down an adapter that no-one is using any 
more, but this assumes that the adapter is still connected to the machine. 
That's not always true for a USB adapter, so disable the sleep operations when 
the adapter has been physically unplugged.

This prevents I2C write failures with error -19 from appearing occasionally in 
the dmesg log.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-disconnect-dont-sleep.diff --]
[-- Type: text/x-patch, Size: 1957 bytes --]

--- linux-3.0/drivers/media/common/tuners/tda18271-fe.c.orig	2011-08-18 16:55:53.000000000 +0100
+++ linux-3.0/drivers/media/common/tuners/tda18271-fe.c	2011-08-18 23:12:55.000000000 +0100
@@ -1230,7 +1230,7 @@
 	return 0;
 }
 
-static struct dvb_tuner_ops tda18271_tuner_ops = {
+static const struct dvb_tuner_ops tda18271_tuner_ops = {
 	.info = {
 		.name = "NXP TDA18271HD",
 		.frequency_min  =  45000000,
--- linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c.orig	2011-08-18 16:56:02.000000000 +0100
+++ linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c	2011-08-18 23:14:06.000000000 +0100
@@ -778,7 +778,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
 
-static struct dvb_frontend_ops cxd2820r_ops[2];
+static const struct dvb_frontend_ops cxd2820r_ops[2];
 
 struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
 	struct i2c_adapter *i2c, struct dvb_frontend *fe)
@@ -844,7 +844,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_attach);
 
-static struct dvb_frontend_ops cxd2820r_ops[2] = {
+static const struct dvb_frontend_ops cxd2820r_ops[2] = {
 	{
 		/* DVB-T/T2 */
 		.info = {
--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-17 08:52:30.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-18 23:17:42.000000000 +0100
@@ -720,6 +720,12 @@
 	goto ret;
 }
 
+static inline void prevent_sleep(struct dvb_frontend_ops *ops) {
+	ops->set_voltage = NULL;
+	ops->sleep = NULL;
+	ops->tuner_ops.sleep = NULL;
+}
+
 static int dvb_fini(struct em28xx *dev)
 {
 	if (!dev->board.has_dvb) {
@@ -728,8 +734,17 @@
 	}
 
 	if (dev->dvb) {
-		unregister_dvb(dev->dvb);
-		kfree(dev->dvb);
+		struct em28xx_dvb *dvb = dev->dvb;
+
+		if (dev->state & DEV_DISCONNECTED) {
+			/* We cannot tell the device to sleep
+			 * once it has been unplugged. */
+			prevent_sleep(&dvb->fe[0]->ops);
+			prevent_sleep(&dvb->fe[1]->ops);
+		}
+
+		unregister_dvb(dvb);
+		kfree(dvb);
 		dev->dvb = NULL;
 	}
 

[PATCH 1/2] EM28xx - fix race on disconnect

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

*Sigh* I overlooked two patches in the original numbering...

This patch closes the race on the device and extension lists at USB disconnect 
time. Previously, the device was removed from the device list during 
em28xx_release_resources(), and then passed to the em28xx_close_extension() 
function so that all extensions could run their fini() operations. However, this 
left a (brief, theoretical, highly unlikely ;-)) window between these two calls 
during which a new module could call em28xx_register_extension(). The result 
would have been that the em28xx_usb_disconnect() function would also have passed 
the device to the new extension's fini() function, despite never having called 
the extension's init() function.

This patch also restores em28xx_close_extension()'s symmetry with 
em28xx_init_extension(), and establishes the property that every device in the 
device list must have been initialised for every extension in the extension list.

Signed-of-by: Chris Rankin <ranki...@yahoo.com>


[-- Attachment #2: EM28xx-race-on-disconnect.diff --]
[-- Type: text/x-patch, Size: 2136 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-19 00:23:17.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-19 00:32:40.000000000 +0100
@@ -2738,9 +2738,9 @@
 #endif /* CONFIG_MODULES */
 
 /*
- * em28xx_realease_resources()
+ * em28xx_release_resources()
  * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
+ * called when the device gets disconnected or at module unload
 */
 void em28xx_release_resources(struct em28xx *dev)
 {
@@ -2754,8 +2754,6 @@
 
 	em28xx_release_analog_resources(dev);
 
-	em28xx_remove_from_devlist(dev);
-
 	em28xx_i2c_unregister(dev);
 
 	v4l2_device_unregister(&dev->v4l2_dev);
@@ -3152,7 +3150,7 @@
 
 /*
  * em28xx_usb_disconnect()
- * called when the device gets diconencted
+ * called when the device gets disconnected
  * video device will be unregistered on v4l2_close in case it is still open
  */
 static void em28xx_usb_disconnect(struct usb_interface *interface)
--- linux-3.0/drivers/media/video/em28xx/em28xx-core.c.orig	2011-08-18 23:07:51.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-core.c	2011-08-19 00:27:00.000000000 +0100
@@ -1160,18 +1160,6 @@
 static DEFINE_MUTEX(em28xx_devlist_mutex);
 
 /*
- * em28xx_realease_resources()
- * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconected or at module unload
-*/
-void em28xx_remove_from_devlist(struct em28xx *dev)
-{
-	mutex_lock(&em28xx_devlist_mutex);
-	list_del(&dev->devlist);
-	mutex_unlock(&em28xx_devlist_mutex);
-};
-
-/*
  * Extension interface
  */
 
@@ -1221,14 +1209,13 @@
 
 void em28xx_close_extension(struct em28xx *dev)
 {
-	struct em28xx_ops *ops = NULL;
+	const struct em28xx_ops *ops = NULL;
 
 	mutex_lock(&em28xx_devlist_mutex);
-	if (!list_empty(&em28xx_extension_devlist)) {
-		list_for_each_entry(ops, &em28xx_extension_devlist, next) {
-			if (ops->fini)
-				ops->fini(dev);
-		}
+	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
+		if (ops->fini)
+			ops->fini(dev);
 	}
+	list_del(&dev->devlist);
 	mutex_unlock(&em28xx_devlist_mutex);
 }

[PATCH 2/2] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

The final patch removes the unplug/replug deadlock by not holding the device 
mutex during dvb_init(). However, this mutex has already been locked during 
device initialisation by em28xx_usb_probe() and is not released again until all 
extensions have been initialised successfully.

The device mutex is not held during either em28xx_register_extension() or 
em28xx_unregister_extension() any more. More importantly, I don't believe it can 
safely be held by these functions because they must both - by their nature - 
acquire the device list mutex before they can iterate through the device list. 
In other words, while usb_probe() and usb_disconnect() acquire the device mutex 
followed by the device list mutex, the register/unregister_extension() functions 
would need to acquire these mutexes in the opposite order. And that sounds like 
a potential deadlock.

On the other hand, the new situation is a definite improvement :-).

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-replug-deadlock.diff --]
[-- Type: text/x-patch, Size: 551 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-19 00:50:41.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-19 00:51:03.000000000 +0100
@@ -542,7 +542,6 @@
 	dev->dvb = dvb;
 	dvb->fe[0] = dvb->fe[1] = NULL;
 
-	mutex_lock(&dev->lock);
 	em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	/* init frontend */
 	switch (dev->model) {
@@ -711,7 +710,6 @@
 	em28xx_info("Successfully loaded em28xx-dvb\n");
 ret:
 	em28xx_set_mode(dev, EM28XX_SUSPEND);
-	mutex_unlock(&dev->lock);
 	return result;
 
 out_free:

Re: [PATCH 6/6] EM28xx - don't sleep on disconnect

[Mauro Carvalho Chehab]

Em 20-08-2011 04:37, Chris Rankin escreveu:
> +
> +		if (dev->state & DEV_DISCONNECTED) {
> +			/* We cannot tell the device to sleep
> +			 * once it has been unplugged. */
> +			prevent_sleep(&dvb->fe[0]->ops);
> +			prevent_sleep(&dvb->fe[1]->ops);

This will cause an OOPS if dvb->fe[n] == NULL.

> +		}
> +

Re: [PATCH 2/2] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Mauro Carvalho Chehab]

Em 20-08-2011 04:46, Chris Rankin escreveu:
> The final patch removes the unplug/replug deadlock by not holding the device mutex during dvb_init(). However, this mutex has already been locked during device initialisation by em28xx_usb_probe() and is not released again until all extensions have been initialised successfully.

No. The extension load can happen after the usb probe phase. In practice,
the only case where the extension init will happen together with the
usb probe phase is when the em28xx modules are compiled builtin, as the
module load is done asynchronously, and generally happens after the em28xx
core to load.

> The device mutex is not held during either em28xx_register_extension() or em28xx_unregister_extension() any more. More importantly, I don't believe it can safely be held by these functions because they must both - by their nature - acquire the device list mutex before they can iterate through the device list. In other words, while usb_probe() and usb_disconnect() acquire the device mutex followed by the device list mutex, the register/unregister_extension() functions would need to acquire these mutexes in the opposite order. And that sounds like a potential deadlock.
> 
> On the other hand, the new situation is a definite improvement :-).

No, it is a regression for a known bug.

This patch can cause troubles. The point is that, after initializing the
analog part, the device can be accessed via the V4L2 API, while it is
still initializing the DVB part. This actually happens in practice, as
when udev detects a new device, it opens it and calls VIDIOC_QUERYCAP.

So, it ends by having a race issue, as at V4L2 open, or at some analog mode
ioctl's, there are calls for em28xx_set_mode(dev, EM28XX_ANALOG_MODE).

In order words, if the DVB initialization is still happening, the driver
should not allow any V4L2 call, otherwise the DVB detection breaks.

Maybe the proper fix would be to change the logic under em28xx_usb_probe()
to not hold dev->lock anymore when the device is loading the extensions.
> 
> Signed-off-by: Chris Rankin <rankincj@yahoo.com>
> 
> 
> EM28xx-replug-deadlock.diff
> 
> 
> --- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-19 00:50:41.000000000 +0100
> +++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-19 00:51:03.000000000 +0100
> @@ -542,7 +542,6 @@
>  	dev->dvb = dvb;
>  	dvb->fe[0] = dvb->fe[1] = NULL;
>  
> -	mutex_lock(&dev->lock);
>  	em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
>  	/* init frontend */
>  	switch (dev->model) {
> @@ -711,7 +710,6 @@
>  	em28xx_info("Successfully loaded em28xx-dvb\n");
>  ret:
>  	em28xx_set_mode(dev, EM28XX_SUSPEND);
> -	mutex_unlock(&dev->lock);
>  	return result;
>  
>  out_free:

Re: [PATCH 1/2] EM28xx - fix race on disconnect

[Sylwester Nawrocki]

Hi Chris,

On 08/20/2011 01:42 PM, Chris Rankin wrote:
> *Sigh* I overlooked two patches in the original numbering...

How about using git format-patch and git send-email ?
http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html

It's easier to review when patches are inlined rather than sent
as an attachment.

git format-patch will make you a nice series of patches. For instance,
when you have 3 of your patches applied on top of some branch and it's
checked out, to generate the patches the simple command:

$ git format-patch -3 --cover-letter

will do. Then you just need to pass the files to 'git send-email'.

Also, the patch description should be wrapped around 75 characters.
So there is no need for text wrapping with 'git log', etc.

Unfortunately git send-email won't work with free e-email accounts
on yahoo.com. The SMTP server throws an error like:
"Access denied : Free users cannot access this server."
Gmail works for me.

--
Regards,
Sylwester

Re: [PATCH 6/6] EM28xx - don't sleep on disconnect

[Chris Rankin]

--- On Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrot
> 
> This will cause an OOPS if dvb->fe[n] == NULL.
> 

OK, that's trivially fixable. I'll send you an updated patch. Is it safe to assume that dvb->fe[0] at least will always be non-NULL?

Cheers,
Chris

Re: [PATCH 6/6] EM28xx - don't sleep on disconnect

[Mauro Carvalho Chehab]

Em 20-08-2011 06:46, Chris Rankin escreveu:
> --- On Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrot
>>
>> This will cause an OOPS if dvb->fe[n] == NULL.
>>
> 
> OK, that's trivially fixable. I'll send you an updated patch. Is it safe to assume that dvb->fe[0] at least will always be non-NULL?

No, it isn't. The dvb initialization may fail or the device can be analog only,
but somebody might manually load em28xx-dvb (or two devices were plugged).

Regards,
Mauro

Re: [PATCH 2/2] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Chris Rankin]

--- On Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> No. The extension load can happen after the usb probe
> phase. In practice, the only case where the extension init will happen
> together with the usb probe phase is when the em28xx modules are
> compiled builtin

It also happens when someone plugs an adapter into the machine when the modules are already loaded. E.g. someone plugging a second adapter in, or unplugging and then replugging the same one.

> Maybe the proper fix would be to change the logic under
> em28xx_usb_probe() to not hold dev->lock anymore when the device is
> loading the extensions.

I could certainly write such a patch, although I only have a PCTV 290e adapter to test with.

Is this problem unique to the em28xx-dvb module? How does the em28xx-alsa module get away with creating ALSA devices without causing a similar race condition?

Cheers,
Chris

Re: [PATCH 2/2] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Mauro Carvalho Chehab]

Em 20-08-2011 07:40, Chris Rankin escreveu:
> --- On Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> No. The extension load can happen after the usb probe
>> phase. In practice, the only case where the extension init will happen
>> together with the usb probe phase is when the em28xx modules are
>> compiled builtin
> 
> It also happens when someone plugs an adapter into the machine when the modules are already loaded. E.g. someone plugging a second adapter in, or unplugging and then replugging the same one.

Yes.

>> Maybe the proper fix would be to change the logic under
>> em28xx_usb_probe() to not hold dev->lock anymore when the device is
>> loading the extensions.
> 
> I could certainly write such a patch, although I only have a PCTV 290e adapter to test with.

We can test it on more devices.

> Is this problem unique to the em28xx-dvb module? How does the em28xx-alsa module get 
> away with creating ALSA devices without causing a similar race condition?

It might also affect alsa. Pulseaudio has the bad habit of opening the device. However,
the device lock is hold when the driver is changing the lock.

It should be noticed that the current mutex lock strategy is a workaround. The proper
solution is to work on resource lock library that could be used when the access of a
device via one API blocks the access of the same device using another API.

We had some discussions about that during the USB mini-summit on last Monday. We'll
probably discuss more about that during the Kernel Summit/media workshop.

Thanks,
Mauro

Re: [PATCH 6/6] EM28xx - don't sleep on disconnect

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 121 bytes --]

Here's the updated patch that checks whether the frontend exists before 
disabling the sleep operations.

Cheers,
Chris


[-- Attachment #2: EM28xx-disconnect-no-sleep.diff --]
[-- Type: text/plain, Size: 2000 bytes --]

--- linux-3.0/drivers/media/common/tuners/tda18271-fe.c.orig	2011-08-20 18:53:48.000000000 +0100
+++ linux-3.0/drivers/media/common/tuners/tda18271-fe.c	2011-08-20 19:21:34.000000000 +0100
@@ -1230,7 +1230,7 @@
 	return 0;
 }
 
-static struct dvb_tuner_ops tda18271_tuner_ops = {
+static const struct dvb_tuner_ops tda18271_tuner_ops = {
 	.info = {
 		.name = "NXP TDA18271HD",
 		.frequency_min  =  45000000,
--- linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c.orig	2011-08-20 18:54:09.000000000 +0100
+++ linux-3.0/drivers/media/dvb/frontends/cxd2820r_core.c	2011-08-20 19:22:21.000000000 +0100
@@ -778,7 +778,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
 
-static struct dvb_frontend_ops cxd2820r_ops[2];
+static const struct dvb_frontend_ops cxd2820r_ops[2];
 
 struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
 	struct i2c_adapter *i2c, struct dvb_frontend *fe)
@@ -844,7 +844,7 @@
 }
 EXPORT_SYMBOL(cxd2820r_attach);
 
-static struct dvb_frontend_ops cxd2820r_ops[2] = {
+static const struct dvb_frontend_ops cxd2820r_ops[2] = {
 	{
 		/* DVB-T/T2 */
 		.info = {
--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig	2011-08-20 18:53:25.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c	2011-08-20 19:26:51.000000000 +0100
@@ -720,6 +720,13 @@
 	goto ret;
 }
 
+static inline void prevent_sleep(struct dvb_frontend_ops *ops)
+{
+	ops->set_voltage = NULL;
+	ops->sleep = NULL;
+	ops->tuner_ops.sleep = NULL;
+}
+
 static int dvb_fini(struct em28xx *dev)
 {
 	if (!dev->board.has_dvb) {
@@ -728,8 +735,19 @@
 	}
 
 	if (dev->dvb) {
-		unregister_dvb(dev->dvb);
-		kfree(dev->dvb);
+		struct em28xx_dvb *dvb = dev->dvb;
+
+		if (dev->state & DEV_DISCONNECTED) {
+			/* We cannot tell the device to sleep
+			 * once it has been unplugged. */
+			if (dvb->fe[0])
+				prevent_sleep(&dvb->fe[0]->ops);
+			if (dvb->fe[1])
+				prevent_sleep(&dvb->fe[1]->ops);
+		}
+
+		unregister_dvb(dvb);
+		kfree(dvb);
 		dev->dvb = NULL;
 	}
 

[PATCH 1/1] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

Here's the new patch for the deadlock problem, which releases the device mutex 
before calling em28xx_init_extension() and then reacquires it afterwards. The 
locking in dvb_init() is now left alone.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-replug-deadlock.diff --]
[-- Type: text/x-patch, Size: 447 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-19 00:45:48.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-20 23:28:39.000000000 +0100
@@ -2929,7 +2929,9 @@
 		goto fail_reg_analog_devices;
 	}
 
+	mutex_unlock(&dev->lock);
 	em28xx_init_extension(dev);
+	mutex_lock(&dev->lock);
 
 	/* Save some power by putting tuner to sleep */
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);

Re: [PATCH 1/1] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Chris Rankin]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

It occurred to me this morning that since we're no longer supposed to be holding 
the device lock when taking the device list lock, then the 
em28xx_usb_disconnect() function needs changing too.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>


[-- Attachment #2: EM28xx-replug-deadlock.diff --]
[-- Type: text/x-patch, Size: 676 bytes --]

--- linux-3.0/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-08-19 00:45:48.000000000 +0100
+++ linux-3.0/drivers/media/video/em28xx/em28xx-cards.c	2011-08-21 13:16:43.000000000 +0100
@@ -2929,7 +2929,9 @@
 		goto fail_reg_analog_devices;
 	}
 
+	mutex_unlock(&dev->lock);
 	em28xx_init_extension(dev);
+	mutex_lock(&dev->lock);
 
 	/* Save some power by putting tuner to sleep */
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
@@ -3191,10 +3193,10 @@
 		em28xx_release_resources(dev);
 	}
 
-	em28xx_close_extension(dev);
-
 	mutex_unlock(&dev->lock);
 
+	em28xx_close_extension(dev);
+
 	if (!dev->users) {
 		kfree(dev->alt_max_pkt_size);
 		kfree(dev);

Re: [PATCH 1/1] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Antti Palosaari]

On 08/21/2011 03:32 PM, Chris Rankin wrote:
> It occurred to me this morning that since we're no longer supposed to be
> holding the device lock when taking the device list lock, then the
> em28xx_usb_disconnect() function needs changing too.
>
> Signed-off-by: Chris Rankin <rankincj@yahoo.com>

I ran that also when re-plugging both PCTV 290e and 460e as today 
LinuxTV 3.2 tree. Seems like this patch is still missing and maybe some 
more.

git log drivers/media/video/em28xx/ |grep -A3 "Author: Chris Rankin"
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Tue Sep 13 12:23:39 2011 +0300

     em28xx: ERROR: "em28xx_add_into_devlist" 
[drivers/media/video/em28xx/em28xx.ko] undefined!
--
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Sat Aug 20 16:01:26 2011 -0300

     [media] em28xx: don't sleep on disconnect
--
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Sat Aug 20 08:31:05 2011 -0300

     [media] em28xx: move printk lines outside mutex lock
--
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Sat Aug 20 08:28:17 2011 -0300

     [media] em28xx: clean up resources should init fail
--
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Sat Aug 20 08:21:03 2011 -0300

     [media] em28xx: use atomic bit operations for devices-in-use mask
--
Author: Chris Rankin <rankincj@yahoo.com>
Date:   Sat Aug 20 08:08:34 2011 -0300

     [media] em28xx: pass correct buffer size to snprintf


regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/1] EM28xx - fix deadlock when unplugging and replugging a DVB adapter

[Chris Rankin]

On 13/09/11 21:04, Antti Palosaari wrote:
> On 08/21/2011 03:32 PM, Chris Rankin wrote:
>> It occurred to me this morning that since we're no longer supposed to be
>> holding the device lock when taking the device list lock, then the
>> em28xx_usb_disconnect() function needs changing too.
>>
>> Signed-off-by: Chris Rankin <rankincj@yahoo.com>
>
> I ran that also when re-plugging both PCTV 290e and 460e as today LinuxTV 3.2
> tree. Seems like this patch is still missing and maybe some more.

There was also this patch, which fixed a couple of memory leaks:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg36432.html

IIRC, the main purpose of the other patch was to delete the 
em28xx_remove_from_devlist() function as well by adding the

     list_del(&dev->devlist);

to the em28xx_close_extension() function instead.

http://www.mail-archive.com/linux-media@vger.kernel.org/msg35783.html

Cheers,
Chris