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

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

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

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +28 ~~-26~~

@@ -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;
 }

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +5 ~~-13~~

@@ -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);
 }

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +2 -2

@@ -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);

Mauro Carvalho Chehab

2011-08-19 05:53 UTC #5 lore raw

Em 18-08-2011 15:11, Chris Rankin escreveu:

Quoted 10 lines

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 <[email protected]>

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:

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

Chris Rankin

author

2011-08-20 11:08 UTC #6 lore raw

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

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

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +1 -1

@@ -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;

Chris Rankin

author

2011-08-20 11:14 UTC #7 lore raw

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

Release the dev->alt_max_pkt_size buffer in all cases.

drivers/media/video/em28xx/em28xx-video.c renamed from drivers/media/video/em28xx/em28xx-video.c.orig +1 -0

@@ -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;
 		}

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +1 -0

@@ -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;

Chris Rankin

author

2011-08-20 11:21 UTC #8 lore raw

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

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.

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +16 ~~-18~~

@@ -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;
 }

Chris Rankin

author

2011-08-20 11:28 UTC #9 lore raw

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

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.

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +28 ~~-26~~

@@ -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;
 }

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +5 ~~-13~~

@@ -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);
 }

Chris Rankin

author

2011-08-20 11:31 UTC #10 lore raw

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

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

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +2 -2

@@ -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);

Chris Rankin

author

2011-08-20 11:37 UTC #11 lore raw

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

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.

drivers/media/common/tuners/tda18271-fe.c renamed from drivers/media/common/tuners/tda18271-fe.c.orig +1 -1

@@ -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,

drivers/media/dvb/frontends/cxd2820r_core.c renamed from drivers/media/dvb/frontends/cxd2820r_core.c.orig +2 -2

@@ -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 = {

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +17 -2

@@ -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;
 	}

Mauro Carvalho Chehab

2011-08-20 12:17 UTC #12 lore raw

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

Em 20-08-2011 04:37, Chris Rankin escreveu:

drivers/media/video/em28xx/em28xx-dvb.c line 743

Quoted 6 lines

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +6 -0

+
+		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.

Quoted 2 lines

(quoted diff) +2 -0
+		}
+

Chris Rankin

author

2011-08-20 13:46 UTC #13 lore raw

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

--- On Sat, 20/8/11, Mauro Carvalho Chehab <[email protected]> wrot

Quoted 3 lines

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

Mauro Carvalho Chehab

2011-08-20 14:20 UTC #14 lore raw

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

Em 20-08-2011 06:46, Chris Rankin escreveu:

Quoted 6 lines

--- On Sat, 20/8/11, Mauro Carvalho Chehab <[email protected]> wrot
Quoted 3 lines
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

Chris Rankin

author

2011-08-20 19:08 UTC #15 lore raw

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

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

Cheers,
Chris

drivers/media/common/tuners/tda18271-fe.c renamed from drivers/media/common/tuners/tda18271-fe.c.orig +1 -1

@@ -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,

drivers/media/dvb/frontends/cxd2820r_core.c renamed from drivers/media/dvb/frontends/cxd2820r_core.c.orig +2 -2

@@ -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 = {

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +20 -2

@@ -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;
 	}

Chris Rankin

author

2011-08-20 11:42 UTC #16 lore raw

PATCH 1/2 EM28xx - fix race on disconnect

*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 <[email protected]>

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +3 -5

@@ -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)

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +5 ~~-18~~

@@ -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);
 }

Sylwester Nawrocki

2011-08-20 12:36 UTC #17 lore raw

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

Hi Chris,

Quoted 1 line · Chris Rankin

*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

Chris Rankin

author

2011-08-20 11:46 UTC #18 lore raw

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

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 :-).

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +0 -2

@@ -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:

Mauro Carvalho Chehab

2011-08-20 12:34 UTC #19 lore raw

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

Em 20-08-2011 04:46, Chris Rankin escreveu:

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 25 lines

EM28xx-replug-deadlock.diff

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +0 -2

@@ -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:

Chris Rankin

author

2011-08-20 14:40 UTC #20 lore raw

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

Quoted 4 lines · Mauro Carvalho Chehab

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.

Quoted 3 lines

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

Mauro Carvalho Chehab

2011-08-20 15:02 UTC #21 lore raw

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

Em 20-08-2011 07:40, Chris Rankin escreveu:

Quoted 7 lines

Quoted 4 lines · Mauro Carvalho Chehab
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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 2 lines

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

Chris Rankin

author

2011-08-20 22:38 UTC #22 lore raw

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

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.

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +2 -0

@@ -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);

Chris Rankin

author

2011-08-21 12:32 UTC #23 lore raw

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

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.

http://www.mail-archive.com/[email protected]/msg36432.html

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +4 -2

@@ -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);

Antti Palosaari

2011-09-13 20:04 UTC #24 lore raw

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

Quoted 5 lines · Chris Rankin

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 <[email protected]>

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 <[email protected]>
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 <[email protected]>
Date:   Sat Aug 20 16:01:26 2011 -0300

[media] em28xx: don't sleep on disconnect
--

Author: Chris Rankin <[email protected]>
Date:   Sat Aug 20 08:31:05 2011 -0300

[media] em28xx: move printk lines outside mutex lock
--

Author: Chris Rankin <[email protected]>
Date:   Sat Aug 20 08:28:17 2011 -0300

[media] em28xx: clean up resources should init fail
--

Author: Chris Rankin <[email protected]>
Date:   Sat Aug 20 08:21:03 2011 -0300

[media] em28xx: use atomic bit operations for devices-in-use mask
--

Author: Chris Rankin <[email protected]>
Date:   Sat Aug 20 08:08:34 2011 -0300

[media] em28xx: pass correct buffer size to snprintf

regards
Antti

Signature · 1 line

http://palosaari.fi/

Chris Rankin

author

2011-09-13 20:47 UTC #25 lore raw

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

Quoted 9 lines · Antti Palosaari

Quoted 5 lines · Chris Rankin
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 <[email protected]>
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:

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/[email protected]/msg35783.html

Cheers,
Chris

Chris Rankin

author

2011-08-18 22:28 UTC #26 lore raw

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.

drivers/media/common/tuners/tda18271-fe.c renamed from drivers/media/common/tuners/tda18271-fe.c.orig +1 -1

@@ -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,

drivers/media/dvb/frontends/cxd2820r_core.c renamed from drivers/media/dvb/frontends/cxd2820r_core.c.orig +2 -2

@@ -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 = {

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +17 -2

@@ -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;
 	}

Chris Rankin

author

2011-08-18 23:45 UTC #27 lore raw

Signed-of-by Chris Rankin <[email protected]>

drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +3 -5

@@ -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)

drivers/media/video/em28xx/em28xx-core.c renamed from drivers/media/video/em28xx/em28xx-core.c.orig +5 ~~-18~~

@@ -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);
 }

Chris Rankin

author

2011-08-19 00:12 UTC #28 lore raw

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

drivers/media/video/em28xx/em28xx-dvb.c renamed from drivers/media/video/em28xx/em28xx-dvb.c.orig +0 -2

@@ -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:

Chris Rankin

author

2011-08-18 20:02 UTC #29 lore raw