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

Cheers,
Chris
  

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html