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

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

[Chris Rankin]

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

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 <ranki...@yahoo.com>

Cheers,
Chris

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

--- linux/drivers/media/video/em28xx/em28xx-cards.c.orig	2011-09-24 21:42:43.000000000 +0100
+++ linux/drivers/media/video/em28xx/em28xx-cards.c	2011-09-24 21:48:56.000000000 +0100
@@ -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);

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

[Mauro Carvalho Chehab]

Em 24-09-2011 17:54, Chris Rankin escreveu:
> 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 <ranki...@yahoo.com>

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

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

[Mauro Carvalho Chehab]

Em 24-09-2011 17:54, Chris Rankin escreveu:
> 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 <ranki...@yahoo.com>

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

From: Mauro Carvalho Chehab <mchehab@redhat.com>
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 <rankincj@yahoo.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 7297d90..c92c177 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -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);
diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index 804a4ab..afddfea 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -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);
 }
 
diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index 1626e4a..a5c1ba2 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -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);
 

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

[Chris Rankin]

On 25/09/11 13:49, Mauro Carvalho Chehab wrote:
> 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.

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

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

[Chris Rankin]

On 25/09/11 14:00, Mauro Carvalho Chehab wrote:
> 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

> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
> index 7297d90..c92c177 100644
> --- a/drivers/media/video/em28xx/em28xx-cards.c
> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> @@ -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);
> diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
> index 804a4ab..afddfea 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -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);
>   }
>
> diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
> index 1626e4a..a5c1ba2 100644
> --- a/drivers/media/video/em28xx/em28xx.h
> +++ b/drivers/media/video/em28xx/em28xx.h
> @@ -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);
>

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

[Chris Rankin]

--- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> 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

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

[Mauro Carvalho Chehab]

Em 25-09-2011 10:32, Chris Rankin escreveu:
> On 25/09/11 14:00, Mauro Carvalho Chehab wrote:
>> 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.

> 
> Cheers,
> Chris
> 
>> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
>> index 7297d90..c92c177 100644
>> --- a/drivers/media/video/em28xx/em28xx-cards.c
>> +++ b/drivers/media/video/em28xx/em28xx-cards.c
>> @@ -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);
>> diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
>> index 804a4ab..afddfea 100644
>> --- a/drivers/media/video/em28xx/em28xx-core.c
>> +++ b/drivers/media/video/em28xx/em28xx-core.c
>> @@ -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);
>>   }
>>
>> diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
>> index 1626e4a..a5c1ba2 100644
>> --- a/drivers/media/video/em28xx/em28xx.h
>> +++ b/drivers/media/video/em28xx/em28xx.h
>> @@ -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);
>>
> 

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

[Mauro Carvalho Chehab]

Em 25-09-2011 16:28, Chris Rankin escreveu:
> --- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> 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()? 

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.

> 
> Cheers,
> Chris
> 

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

[Chris Rankin]

--- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> 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