EM28xx - fix deadlock when unplugging and replugging a DVB adapter

PATCH In review Chris Rankin 2 participants 9 messages Sep 24, 2011 – Sep 25, 2011 Media (V4L/DVB)

lore

Share & tools

lore

Chris Rankin

author

2011-09-24 20:55 UTC #0 lore raw

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

Mauro,

Excuse me - I put my brain in backwards today and sent you a reverse diff by accident! Reresending...

----------------------------------------------------------------------------
This fixes the deadlock that occurs with either multiple PCTV 290e adapters or when a single PCTV 290e adapter is replugged.

For DVB devices, the device lock must now *not* be held when adding/removing either a device or an extension to the respective lists. (Because em28xx_init_dvb() will want to take the lock instead).

Conversely, for Audio-Only devices, the device lock *must* be held when adding/removing either a device or an extension to the respective lists.

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

Cheers,
Chris

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

@@ -3005,7 +3005,9 @@
 		goto fail;
 	}
 
+	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);

@@ -3301,10 +3303,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);

Mauro Carvalho Chehab

2011-09-25 12:49 UTC #1 lore raw

Em 24-09-2011 17:54, Chris Rankin escreveu:

Quoted 7 lines

This fixes the deadlock that occurs with either multiple PCTV 290e adapters or when a single PCTV 290e adapter is replugged.
For DVB devices, the device lock must now *not* be held when adding/removing either a device or an extension to the respective lists. (Because em28xx_init_dvb() will want to take the lock instead).
Conversely, for Audio-Only devices, the device lock *must* be held when adding/removing either a device or an extension to the respective lists.
Signed-off-by Chris Rankin <[email protected]>

Ok, I've applied it, but it doesn't sound a good idea to me to do:

+ mutex_unlock(&dev->lock);
em28xx_init_extension(dev);
+ mutex_lock(&dev->lock);

I'll later test it with some hardware and see how well this behaves in practice.

Thanks,
Mauro

Chris Rankin

author

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

Quoted 8 lines · Mauro Carvalho Chehab

Ok, I've applied it, but it doesn't sound a good idea to me to do:
drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +2 -0
+	mutex_unlock(&dev->lock);
  	em28xx_init_extension(dev);
+	mutex_lock(&dev->lock);
I'll later test it with some hardware and see how well this behaves in practice.

No, I don't like it either. But since a kernel mutex isn't recursive, I can't think of a better solution at the moment.

Cheers,
Chris

Chris Rankin

author

2011-09-25 19:28 UTC #3 lore raw

In reply to Mauro Carvalho Chehab

Quoted 7 lines · Mauro Carvalho Chehab

Ok, I've applied it, but it doesn't sound a good idea to me to do:
+    mutex_unlock(&dev->lock);
    em28xx_init_extension(dev);
+    mutex_lock(&dev->lock);

Yes, I suppose it's the logical equivalent of moving the em28xx_init_extension(dev) call from em28xx_init_dev(), and placing it immediately after the final mutex_unlock(&dev->lock) call in em28xx_usb_probe() instead. Which would be cleaner, quite frankly.

Which stage of the v4l2 initialisation triggers the race with udev? v4l2_device_register()?

Cheers,
Chris

Mauro Carvalho Chehab

2011-09-25 19:45 UTC #4 lore raw

Em 25-09-2011 16:28, Chris Rankin escreveu:

Quoted 4 lines

Quoted 3 lines · Mauro Carvalho Chehab
Ok, I've applied it, but it doesn't sound a good idea to me to do:

Quoted 8 lines

drivers/media/video/em28xx/em28xx-cards.c line 3010
Quoted 4 lines
drivers/media/video/em28xx/em28xx-cards.c renamed from drivers/media/video/em28xx/em28xx-cards.c.orig +2 -0
+    mutex_unlock(&dev->lock);
     em28xx_init_extension(dev);
+    mutex_lock(&dev->lock);
Yes, I suppose it's the logical equivalent of moving the em28xx_init_extension(dev) call from em28xx_init_dev(), and placing it immediately after the final mutex_unlock(&dev->lock) call in em28xx_usb_probe() instead. Which would be cleaner, quite frankly.
Which stage of the v4l2 initialisation triggers the race with udev? v4l2_device_register()?

Yes. Just after creating a device, udev tries to access it. This bug is more sensitive on multi-CPU machines, as udev may run on another CPU.

Quoted 4 lines

Cheers,
Chris

Chris Rankin

author

2011-09-25 21:36 UTC #5 lore raw

Quoted 3 lines · Mauro Carvalho Chehab

Yes. Just after creating a device, udev tries to access it. This bug is more sensitive on multi-CPU machines, as udev may run on another CPU.

Heh, I have a hyper-threaded quad-core here. I suspect that counts as "multi-CPU" :-).

However, I think we can agree that the first "plugging" event is not causing problems (in the modular case). The interesting thing about this first event is that it requests that the em28xx_dvb module be loaded, which in turn means that em28xx_init_extension() cannot invoke the dvb_init() function during em28xx_usb_probe(), thus avoiding the deadlock. So one of the following sequences of events must be occurring instead:

Either:
1) em28xx_usb_probe() runs
2) em28xx_dvb module loads, invoking em28xx_register_extension() and dvb_init()
3) udev runs for V4L nodes

Or:
1) em28xx_usb_probe() runs
2) udev runs for V4L nodes
3) em28xx_dvb module loads, invoking em28xx_register_extension() and dvb_init()

The steps in both of these sequences are serialised by the dev->lock mutex. So wouldn't moving em28xx_init_extension() out of em28xx_init_dev() to the bottom of em28xx_usb_probe() (after the dev->lock mutex has been unlocked) be logically identical to the case where the em28xx-dvb module is loaded?

Cheers,
Chris

Mauro Carvalho Chehab

2011-09-25 13:00 UTC #6 lore raw

In reply to Chris Rankin

Em 24-09-2011 17:54, Chris Rankin escreveu:

Quoted 7 lines

This fixes the deadlock that occurs with either multiple PCTV 290e adapters or when a single PCTV 290e adapter is replugged.
For DVB devices, the device lock must now *not* be held when adding/removing either a device or an extension to the respective lists. (Because em28xx_init_dvb() will want to take the lock instead).
Conversely, for Audio-Only devices, the device lock *must* be held when adding/removing either a device or an extension to the respective lists.
Signed-off-by Chris Rankin <[email protected]>

Hmm... This would probably work better (not tested). Could you please test it on your hardware?

From: Mauro Carvalho Chehab <[email protected]>
Date: Sat, 24 Sep 2011 16:54:58 -0300
Subject: [media] em28xx: fix deadlock when unplugging and replugging a DVB adapter

This fixes the deadlock that occurs with either multiple PCTV 290e adapters or when a single PCTV 290e adapter is replugged.

For DVB devices, the device lock must now *not* be held when adding/removing either a device or an extension to the respective lists. (Because em28xx_init_dvb() will want to take the lock instead).

Conversely, for Audio-Only devices, the device lock *must* be held when adding/removing either a device or an extension to the respective lists.

Based on a patch from Chris Rankin <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/video/em28xx/em28xx-cards.c +4 -3

index 7297d90..c92c177 100644
@@ -3005,7 +3005,8 @@ static int em28xx_init_dev(struct em28xx **devhandle, struct usb_device *udev,
 		goto fail;
 	}
 
-	em28xx_init_extension(dev);
+	/* dev->lock needs to be holded */
+	__em28xx_init_extension(dev);
 
 	/* Save some power by putting tuner to sleep */
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);

@@ -3301,10 +3302,10 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 		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);

drivers/media/video/em28xx/em28xx-core.c +8 -2

index 804a4ab..afddfea 100644
@@ -1218,16 +1218,22 @@ void em28xx_unregister_extension(struct em28xx_ops *ops)
 }
 EXPORT_SYMBOL(em28xx_unregister_extension);
 
-void em28xx_init_extension(struct em28xx *dev)
+/* Need to take the mutex lock before calling it */
+void __em28xx_init_extension(struct em28xx *dev)
 {
 	const struct em28xx_ops *ops = NULL;
 
-	mutex_lock(&em28xx_devlist_mutex);
 	list_add_tail(&dev->devlist, &em28xx_devlist);
 	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
 		if (ops->init)
 			ops->init(dev);
 	}
+}
+
+void em28xx_init_extension(struct em28xx *dev)
+{
+	mutex_lock(&em28xx_devlist_mutex);
+	__em28xx_init_extension(dev);
 	mutex_unlock(&em28xx_devlist_mutex);
 }

drivers/media/video/em28xx/em28xx.h +1 -0

index 1626e4a..a5c1ba2 100644
@@ -682,6 +682,7 @@ void em28xx_remove_from_devlist(struct em28xx *dev);
 void em28xx_add_into_devlist(struct em28xx *dev);
 int em28xx_register_extension(struct em28xx_ops *dev);
 void em28xx_unregister_extension(struct em28xx_ops *dev);
+void __em28xx_init_extension(struct em28xx *dev);
 void em28xx_init_extension(struct em28xx *dev);
 void em28xx_close_extension(struct em28xx *dev);

Chris Rankin

author

2011-09-25 13:32 UTC #7 lore raw

Quoted 2 lines · Mauro Carvalho Chehab

Hmm... This would probably work better (not tested). Could you please test it on your hardware?

Hmm, I don't understand this. The deadlock isn't about taking em28xx_devlist_mutex, but happens because em28xx_dvb_init() tries to retake dev->lock when em28xx_usb_probe() is already holding it. That's why I unlocked dev->lock before calling em28xx_init_extension().

So why are you avoiding locking em28xx_devlist_mutex?

Cheers,
Chris

Quoted 68 lines

drivers/media/video/em28xx/em28xx-cards.c +4 -3

index 7297d90..c92c177 100644
@@ -3005,7 +3005,8 @@ static int em28xx_init_dev(struct em28xx **devhandle, struct usb_device *udev,
  		goto fail;
  	}
 
-	em28xx_init_extension(dev);
+	/* dev->lock needs to be holded */
+	__em28xx_init_extension(dev);
 
  	/* Save some power by putting tuner to sleep */
  	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);

@@ -3301,10 +3302,10 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
  		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);

drivers/media/video/em28xx/em28xx-core.c +8 -2

index 804a4ab..afddfea 100644
@@ -1218,16 +1218,22 @@ void em28xx_unregister_extension(struct em28xx_ops *ops)
  }
  EXPORT_SYMBOL(em28xx_unregister_extension);
 
-void em28xx_init_extension(struct em28xx *dev)
+/* Need to take the mutex lock before calling it */
+void __em28xx_init_extension(struct em28xx *dev)
  {
  	const struct em28xx_ops *ops = NULL;
 
-	mutex_lock(&em28xx_devlist_mutex);
  	list_add_tail(&dev->devlist,&em28xx_devlist);
  	list_for_each_entry(ops,&em28xx_extension_devlist, next) {
  		if (ops->init)
  			ops->init(dev);
  	}
+}
+
+void em28xx_init_extension(struct em28xx *dev)
+{
+	mutex_lock(&em28xx_devlist_mutex);
+	__em28xx_init_extension(dev);
  	mutex_unlock(&em28xx_devlist_mutex);
  }

drivers/media/video/em28xx/em28xx.h +1 -0

index 1626e4a..a5c1ba2 100644
@@ -682,6 +682,7 @@ void em28xx_remove_from_devlist(struct em28xx *dev);
  void em28xx_add_into_devlist(struct em28xx *dev);
  int em28xx_register_extension(struct em28xx_ops *dev);
  void em28xx_unregister_extension(struct em28xx_ops *dev);
+void __em28xx_init_extension(struct em28xx *dev);
  void em28xx_init_extension(struct em28xx *dev);
  void em28xx_close_extension(struct em28xx *dev);

Mauro Carvalho Chehab

2011-09-25 19:43 UTC #8 lore raw

Em 25-09-2011 10:32, Chris Rankin escreveu:

Quoted 8 lines · Mauro Carvalho Chehab

Quoted 2 lines
Hmm... This would probably work better (not tested). Could you please test it on your hardware?
Hmm, I don't understand this.
The deadlock isn't about taking em28xx_devlist_mutex, but happens because em28xx_dvb_init() tries to retake dev->lock when em28xx_usb_probe() is already holding it. That's why I unlocked dev->lock before calling em28xx_init_extension().
So why are you avoiding locking em28xx_devlist_mutex?

Yeah, I realized it just after sending it. I didn't have time to re-post another email, as I was about to leave.

I'm still not comfortable on releasing the mutex too early, as this may cause driver hungup, due to udev early access. I'll try to do some tests with it here, and think on a better solution.

Quoted 73 lines

Cheers,
Chris

Quoted 68 lines

drivers/media/video/em28xx/em28xx-cards.c +4 -3

index 7297d90..c92c177 100644
@@ -3005,7 +3005,8 @@ static int em28xx_init_dev(struct em28xx **devhandle, struct usb_device *udev,
          goto fail;
      }
 
-    em28xx_init_extension(dev);
+    /* dev->lock needs to be holded */
+    __em28xx_init_extension(dev);
 
      /* Save some power by putting tuner to sleep */
      v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);

@@ -3301,10 +3302,10 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
          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);

drivers/media/video/em28xx/em28xx-core.c +8 -2

index 804a4ab..afddfea 100644
@@ -1218,16 +1218,22 @@ void em28xx_unregister_extension(struct em28xx_ops *ops)
  }
  EXPORT_SYMBOL(em28xx_unregister_extension);
 
-void em28xx_init_extension(struct em28xx *dev)
+/* Need to take the mutex lock before calling it */
+void __em28xx_init_extension(struct em28xx *dev)
  {
      const struct em28xx_ops *ops = NULL;
 
-    mutex_lock(&em28xx_devlist_mutex);
      list_add_tail(&dev->devlist,&em28xx_devlist);
      list_for_each_entry(ops,&em28xx_extension_devlist, next) {
          if (ops->init)
              ops->init(dev);
      }
+}
+
+void em28xx_init_extension(struct em28xx *dev)
+{
+    mutex_lock(&em28xx_devlist_mutex);
+    __em28xx_init_extension(dev);
      mutex_unlock(&em28xx_devlist_mutex);
  }

drivers/media/video/em28xx/em28xx.h +1 -0

index 1626e4a..a5c1ba2 100644
@@ -682,6 +682,7 @@ void em28xx_remove_from_devlist(struct em28xx *dev);
  void em28xx_add_into_devlist(struct em28xx *dev);
  int em28xx_register_extension(struct em28xx_ops *dev);
  void em28xx_unregister_extension(struct em28xx_ops *dev);
+void __em28xx_init_extension(struct em28xx *dev);
  void em28xx_init_extension(struct em28xx *dev);
  void em28xx_close_extension(struct em28xx *dev);