Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

PATCH In review Hans de Goede 5 participants 9 messages Jun 9, 2011 – Jun 16, 2011 Media (V4L/DVB)

lore

Share & tools

lore

Hans de Goede

author

2011-06-09 11:21 UTC #0 lore raw

Hi,

When I unplug a uvc camera *while streaming* I get:

Jun  9 13:20:02 shalem kernel: [15824.809741] BUG: unable to handle kernel NULL pointer dereference at           (null)
Jun  9 13:20:02 shalem kernel: [15824.809816] IP: [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.809877] PGD 0
Jun  9 13:20:02 shalem kernel: [15824.809898] Oops: 0000 [#1] SMP
Jun  9 13:20:02 shalem kernel: [15824.809933] CPU 1
Jun  9 13:20:02 shalem kernel: [15824.809952] Modules linked in: uvcvideo videodev media v4l2_compat_ioctl32 nf_conntrack_ipv4 nf_defrag_ipv4 vfat fat tcp_lp tun fuse ebtable_nat ebtables cpufreq_ondemand acpi_cpufreq freq_table mperf bridge stp llc be2iscsi iscsi_boot_sysfs bnx2i cnic uio cxgb3i libcxgbi iw_cxgb3 cxgb3 mdio ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi ip6t_REJECT nf_conntrack_ipv6 coretemp xt_physdev nf_defrag_ipv6 ip6table_filter xt_state ip6_tables nf_conntrack snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_bt87x usb_storage snd_seq snd_usb_audio uas snd_pcm snd_hwdep snd_usbmidi_lib ppdev snd_rawmidi microcode e1000e snd_seq_device serio_raw i2c_i801 snd_timer tpm_infineon snd iTCO_wdt parport_pc parport soundcore mei(C) iTCO_vendor_support snd_page_alloc virtio_net kvm_intel kvm ipv6 i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: tuner_types]
Jun  9 13:20:02 shalem kernel: [15824.810794]
Jun  9 13:20:02 shalem kernel: [15824.810811] Pid: 4944, comm: camorama Tainted: G         C  3.0.0-rc1+ #5 FUJITSU D3071-S1/D3071-S1
Jun  9 13:20:02 shalem kernel: [15824.810888] RIP: 0010:[<ffffffffa0309eae>]  [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.810961] RSP: 0018:ffff88011da23d98  EFLAGS: 00010286
Jun  9 13:20:02 shalem kernel: [15824.811003] RAX: 0000000000000000 RBX: ffff88006a864400 RCX: 0000000000000118
Jun  9 13:20:02 shalem kernel: [15824.811057] RDX: 0000000000000001 RSI: 0000000000000004 RDI: ffff88006a864400
Jun  9 13:20:02 shalem kernel: [15824.811112] RBP: ffff88011da23d98 R08: ffffea0002896e28 R09: ffff88011da23d38
Jun  9 13:20:02 shalem kernel: [15824.811165] R10: ffffffffa0526026 R11: ffffffff81a58210 R12: ffff880027d43840
Jun  9 13:20:02 shalem kernel: [15824.811219] R13: 0000000000000008 R14: ffff880133310dc0 R15: ffff88012db76300
Jun  9 13:20:02 shalem kernel: [15824.811274] FS:  00007f7942ff49c0(0000) GS:ffff88013e280000(0000) knlGS:0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.811336] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jun  9 13:20:02 shalem kernel: [15824.811381] CR2: 0000000000000000 CR3: 0000000001a03000 CR4: 00000000000406e0
Jun  9 13:20:02 shalem kernel: [15824.811435] DR0: 0000000000000003 DR1: 00000000000000b0 DR2: 0000000000000001
Jun  9 13:20:02 shalem kernel: [15824.811491] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Jun  9 13:20:02 shalem kernel: [15824.811546] Process camorama (pid: 4944, threadinfo ffff88011da22000, task ffff880113618000)
Jun  9 13:20:02 shalem kernel: [15824.811609] Stack:
Jun  9 13:20:02 shalem kernel: [15824.811629]  ffff88011da23db8 ffffffffa0510203 ffff880027d43840 ffff880133310dc0
Jun  9 13:20:02 shalem kernel: [15824.811695]  ffff88011da23e08 ffffffff811233fe ffff880027d43850 ffff880134530500
Jun  9 13:20:02 shalem kernel: [15824.811763]  ffffffff811e3397 ffff880027d43840 ffff8801347a5b80 0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.811829] Call Trace:
Jun  9 13:20:02 shalem kernel: [15824.811856]  [<ffffffffa0510203>] v4l2_release+0x7b/0x8e [videodev]
Jun  9 13:20:02 shalem kernel: [15824.811910]  [<ffffffff811233fe>] fput+0x121/0x1e3
Jun  9 13:20:02 shalem kernel: [15824.811953]  [<ffffffff811e3397>] ? exit_sem+0x1c7/0x1d8
Jun  9 13:20:02 shalem kernel: [15824.811998]  [<ffffffff811208a7>] filp_close+0x6e/0x7a
Jun  9 13:20:02 shalem kernel: [15824.812041]  [<ffffffff81131b33>] ? __d_free+0x53/0x58
Jun  9 13:20:02 shalem kernel: [15824.812084]  [<ffffffff810567e1>] put_files_struct+0x6e/0xd5
Jun  9 13:20:02 shalem kernel: [15824.812130]  [<ffffffff810568d5>] exit_files+0x41/0x46
Jun  9 13:20:02 shalem kernel: [15824.812172]  [<ffffffff81056e55>] do_exit+0x2aa/0x738
Jun  9 13:20:02 shalem kernel: [15824.812214]  [<ffffffff8104be73>] ? wake_up_state+0x10/0x12
Jun  9 13:20:02 shalem kernel: [15824.812259]  [<ffffffff81062ba2>] ? signal_wake_up+0x32/0x43
Jun  9 13:20:02 shalem kernel: [15824.812304]  [<ffffffff810638b4>] ? zap_other_threads+0x59/0x82
Jun  9 13:20:02 shalem kernel: [15824.812352]  [<ffffffff81057568>] do_group_exit+0x7a/0xa2
Jun  9 13:20:02 shalem kernel: [15824.812395]  [<ffffffff810575a7>] sys_exit_group+0x17/0x17
Jun  9 13:20:02 shalem kernel: [15824.812440]  [<ffffffff81487802>] system_call_fastpath+0x16/0x1b
Jun  9 13:20:02 shalem kernel: [15824.812486] Code: 10 66 41 ff 44 24 40 48 83 c4 18 44 89 f0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 55 48 89 e5 66 66 66 66 90 48 85 ff 74 1c 48 8b 47 10
Jun  9 13:20:02 shalem kernel: [15824.812783] RIP  [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.812839]  RSP <ffff88011da23d98>
Jun  9 13:20:02 shalem kernel: [15824.812867] CR2: 0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.873494] ---[ end trace bfc278787db8cbfb ]---
Jun  9 13:20:02 shalem kernel: [15824.873496] Fixing recursive fault but reboot is needed!

I've not tested if this also impacts 3.0!!

I also get the following during building linuxtv/staging/for_v3.1:

CC [M] drivers/media/video/uvc/uvc_entity.o
drivers/media/video/uvc/uvc_entity.c: In function ‘uvc_mc_register_entities’:
drivers/media/video/uvc/uvc_entity.c:110:6: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

Regards,

Hans

Laurent Pinchart

2011-06-11 09:16 UTC #1 lore raw

Hi Hans,

Quoted 6 lines · On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:

Hi,
When I unplug a uvc camera *while streaming* I get:
[15824.809741] BUG: unable to handle kernel NULL pointer dereference at          
(null)

[snip]

Quoted 1 line

I've not tested if this also impacts 3.0!!

It probably does. Thanks for the report. I'll fix it.

Quoted 7 lines

I also get the following during building linuxtv/staging/for_v3.1:
CC [M] drivers/media/video/uvc/uvc_entity.o
drivers/media/video/uvc/uvc_entity.c: In function ‘uvc_mc_register_entities’: drivers/media/video/uvc/uvc_entity.c:110:6:
warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

Mauro sent a patch to fix that.

Signature · 3 lines

Regards,

Laurent Pinchart

Laurent Pinchart

2011-06-13 09:41 UTC #2 lore raw

Hi Hans (and Hans),

Quoted 5 lines · On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:

Quoted 4 lines · On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
Hi,
When I unplug a uvc camera *while streaming* I get:

Quoted 8 lines

Quoted 2 lines
[15824.809741] BUG: unable to handle kernel NULL pointer dereference at
(null)
[snip]
Quoted 1 line
I've not tested if this also impacts 3.0!!
It probably does. Thanks for the report. I'll fix it.

It does. Fixing the problem turns to be more complex than expected.

The crash is caused by media entities life time management issues.

Entities associated with video device nodes are unregistered in video_unregister_device(). This removes the entity from its parent's entities list, and sets the entity's parent to NULL.

Entities also get/put references to their parent's module through media_entity_get() and media_entity_put(). Those functions are called in the open and release handlers of video device nodes and subdev device nodes. The reason behind this is to avoid a parent module from being removed while a subdev is opened, as closing a subdev can call to the parent's module through board code.

When a UVC device is unplugged while streaming, the uvcvideo driver will call video_unregister_device() in the disconnect handler. This will in turn call media_device_unregister_entity() which sets the entity's parent to NULL. When the user then closes open video device nodes, v4l2_release() calls media_entity_put() which tries to dereference entity->parent, and oopses.

I've tried to move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() (called when the last reference to the video device is released). media_entity_put() is then called before the entity is unregistered, but that results in a different oops: as this happens after the USB disconnect callback is called, entity->parent->dev-

Quoted 1 line

driver is now NULL, and trying to access entity->parent->dev->driver->owner

to decrement the module use count oopses.

One possible workaround is to remove media_entity_get()/media_entity_put() calls from v4l2-dev.c. As the original purpose of those functions was to avoid a parent module from being removed while still accessible through board code, and all existing MC-enabled drivers register video device nodes with the owner equal to the entity's parent's module, we can safely do it.

I'd rather implement a proper solution though, but that's not straightforward. We short-circuit the kernel reference management by going through board code. There's something fundamentally wrong in the way we manage subdevs and device/module reference counts. I'm not sure where the proper fix should go to though.

Signature · 3 lines

Regards,

Laurent Pinchart

Hans Verkuil

2011-06-13 11:11 UTC #3 lore raw

Quoted 52 lines · On Monday, June 13, 2011 11:41:42 Laurent Pinchart wrote:

Hi Hans (and Hans),
Quoted 5 lines · On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:
Quoted 4 lines · On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
Hi,
When I unplug a uvc camera *while streaming* I get:
Quoted 8 lines
Quoted 2 lines
[15824.809741] BUG: unable to handle kernel NULL pointer dereference at
(null)
[snip]
Quoted 1 line
I've not tested if this also impacts 3.0!!
It probably does. Thanks for the report. I'll fix it.
It does. Fixing the problem turns to be more complex than expected.
The crash is caused by media entities life time management issues.
Entities associated with video device nodes are unregistered in video_unregister_device(). This removes the entity from its parent's entities list, and sets the entity's parent to NULL.
Entities also get/put references to their parent's module through media_entity_get() and media_entity_put(). Those functions are called in the open and release handlers of video device nodes and subdev device nodes. The reason behind this is to avoid a parent module from being removed while a subdev is opened, as closing a subdev can call to the parent's module through board code.
When a UVC device is unplugged while streaming, the uvcvideo driver will call video_unregister_device() in the disconnect handler. This will in turn call media_device_unregister_entity() which sets the entity's parent to NULL. When the user then closes open video device nodes, v4l2_release() calls media_entity_put() which tries to dereference entity->parent, and oopses.
I've tried to move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() (called when the last reference to the video device is released). media_entity_put() is then called before the entity is unregistered, but that results in a different oops: as this happens after the USB disconnect callback is called, entity->parent->dev-
Quoted 1 line
driver is now NULL, and trying to access entity->parent->dev->driver->owner
to decrement the module use count oopses.
One possible workaround is to remove media_entity_get()/media_entity_put() calls from v4l2-dev.c. As the original purpose of those functions was to avoid a parent module from being removed while still accessible through board code, and all existing MC-enabled drivers register video device nodes with the owner equal to the entity's parent's module, we can safely do it.

Quoted 5 lines

I'd rather implement a proper solution though, but that's not straightforward. We short-circuit the kernel reference management by going through board code. There's something fundamentally wrong in the way we manage subdevs and device/module reference counts. I'm not sure where the proper fix should go to though.

Hmm. Tricky.

media_device_unregister_entity() is definitely called in the wrong place. It should move to v4l2_device_release(). I wonder why media_entity_get/put are called in v4l2_open and v4l2_release. That should be in __video_register_device and v4l2_device_release as far as I can see.

Just closing a device node shouldn't be cause for changing the module's refcount, that device node registration and the release after unregistration.

I also wonder whether instead of refcounting the module in media_entity_get/put you should refcount the device (entity->parent->dev).

I have to admit that I don't quite understand why the USB disconnect zeroes entity->parent->dev->driver.

I hope this gives some ideas...

Regards,

Hans

Laurent Pinchart

2011-06-13 16:09 UTC #4 lore raw

Hi Hans,

Quoted 63 lines · On Monday 13 June 2011 13:10:57 Hans Verkuil wrote:

Quoted 56 lines · On Monday, June 13, 2011 11:41:42 Laurent Pinchart wrote:
Quoted 5 lines · On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:
Quoted 4 lines · On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
Hi,
When I unplug a uvc camera *while streaming* I get:
Quoted 8 lines
Quoted 2 lines
[15824.809741] BUG: unable to handle kernel NULL pointer dereference
at (null)
[snip]
Quoted 1 line
I've not tested if this also impacts 3.0!!
It probably does. Thanks for the report. I'll fix it.
It does. Fixing the problem turns to be more complex than expected.
The crash is caused by media entities life time management issues.
Entities associated with video device nodes are unregistered in video_unregister_device(). This removes the entity from its parent's entities list, and sets the entity's parent to NULL.
Entities also get/put references to their parent's module through media_entity_get() and media_entity_put(). Those functions are called in the open and release handlers of video device nodes and subdev device nodes. The reason behind this is to avoid a parent module from being removed while a subdev is opened, as closing a subdev can call to the parent's module through board code.
When a UVC device is unplugged while streaming, the uvcvideo driver will call video_unregister_device() in the disconnect handler. This will in turn call media_device_unregister_entity() which sets the entity's parent to NULL. When the user then closes open video device nodes, v4l2_release() calls media_entity_put() which tries to dereference entity->parent, and oopses.
I've tried to move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() (called when the last reference to the video device is released). media_entity_put() is then called before the entity is unregistered, but that results in a different oops: as this happens after the USB disconnect callback is called, entity->parent->dev-
Quoted 2 lines
driver is now NULL, and trying to access
entity->parent->dev->driver->owner
to decrement the module use count oopses.
One possible workaround is to remove media_entity_get()/media_entity_put() calls from v4l2-dev.c. As the original purpose of those functions was to avoid a parent module from being removed while still accessible through board code, and all existing MC-enabled drivers register video device nodes with the owner equal to the entity's parent's module, we can safely do it.
Quoted 5 lines
I'd rather implement a proper solution though, but that's not straightforward. We short-circuit the kernel reference management by going through board code. There's something fundamentally wrong in the way we manage subdevs and device/module reference counts. I'm not sure where the proper fix should go to though.

Quoted 4 lines

Hmm. Tricky.
media_device_unregister_entity() is definitely called in the wrong place. It should move to v4l2_device_release().

Agreed.

Quoted 7 lines

I wonder why media_entity_get/put are called in v4l2_open and v4l2_release. That should be in __video_register_device and v4l2_device_release as far as I can see.
Just closing a device node shouldn't be cause for changing the module's refcount, that device node registration and the release after unregistration.

If you moved media_entity_get/put in the registration and unregistration functions, removing the uvcvideo module would become completely impossible if a driver is plugged in. uvcvideo registers a video device node, which would then increase the refcount on the module and lock it in in memory until the device is unplugged.

Quoted 2 lines

I also wonder whether instead of refcounting the module in media_entity_get/put you should refcount the device (entity->parent->dev).

Increasing the device parent won't prevent the module from being unloaded. You can unload a module even if devices are bound to it.

Quoted 4 lines

I have to admit that I don't quite understand why the USB disconnect zeroes entity->parent->dev->driver.
I hope this gives some ideas...

The easiest workaround is still to remove the media_entity_get()/put() calls from v4l2-dev.c. Those function have been introduced to fix a subdev-specific problem and are used for video nodes as well, but they're not needed there for now. This isn't a long-term fix though.

Signature · 3 lines

Regards,

Laurent Pinchart

Laurent Pinchart

2011-06-15 08:36 UTC #5 lore raw

PATCH 1/1 v4l: Don't access media entity after is has been destroyed

Entities associated with video device nodes are unregistered in video_unregister_device(). This destroys the entity even though it can still be accessed through open video device nodes.

Move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() to ensure that the entity isn't unregistered until the last reference to the video device is released.

Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those functions were designed for subdevs, to avoid a parent module from being removed while still accessible through board code. They're not currently needed for video device nodes, and will oops when a hotpluggable device is disconnected during streaming, as media_entity_put() called in v4l2_device_release() tries to access entity->parent->dev->driver which is set to NULL when the device is disconnected.

Signed-off-by Laurent Pinchart <[email protected]>

---
 drivers/media/video/v4l2-dev.c |   36 +++++++-----------------------------
 1 files changed, 7 insertions(+), 29 deletions(-)

drivers/media/video/v4l2-dev.c +7 ~~-29~~

index 19d5ae2..5dc1fa1 100644
@@ -167,6 +167,12 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
+	    vdev->vfl_type != VFL_TYPE_SUBDEV)
+		media_device_unregister_entity(&vdev->entity);
+#endif
+
 	/* Release video_device and perform other
 	   cleanups as needed. */
 	vdev->release(vdev);

@@ -405,17 +411,6 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 	/* and increase the device refcount */
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
-		entity = media_entity_get(&vdev->entity);
-		if (!entity) {
-			ret = -EBUSY;
-			video_put(vdev);
-			return ret;
-		}
-	}
-#endif
 	if (vdev->fops->open) {
 		if (vdev->lock && mutex_lock_interruptible(vdev->lock)) {
 			ret = -ERESTARTSYS;

@@ -431,14 +426,8 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 
 err:
 	/* decrease the refcount in case of an error */
-	if (ret) {
-#if defined(CONFIG_MEDIA_CONTROLLER)
-		if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-		    vdev->vfl_type != VFL_TYPE_SUBDEV)
-			media_entity_put(entity);
-#endif
+	if (ret)
 		video_put(vdev);
-	}
 	return ret;
 }

@@ -455,11 +444,6 @@ static int v4l2_release(struct inode *inode, struct file *filp)
 		if (vdev->lock)
 			mutex_unlock(vdev->lock);
 	}
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_entity_put(&vdev->entity);
-#endif
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	video_put(vdev);

@@ -754,12 +738,6 @@ void video_unregister_device(struct video_device *vdev)
 	if (!vdev || !video_is_registered(vdev))
 		return;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_device_unregister_entity(&vdev->entity);
-#endif
-
 	mutex_lock(&videodev_lock);
 	/* This must be in a critical section to prevent a race with v4l2_open.
 	 * Once this bit has been cleared video_get may never be called again.

Signature · 1 line

1.7.3.4

Laurent Pinchart

2011-06-15 08:37 UTC #6 lore raw

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

Mauro,

I forgot to mention in the subject line that this fix should go to 3.0. It fixes a kernel oops when a UVC webcam is disconnected during streaming.

I'll send a pull request in the next few days after getting an ack on this patch.

Hans, could you please test if it fixes your issue ?

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

Acked-by on v1 1/1

2011-06-15 17:23 UTC #7 lore raw

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

In reply to Laurent Pinchart

Quoted 9 lines · Laurent Pinchart

Entities associated with video device nodes are unregistered in video_unregister_device(). This destroys the entity even though it can still be accessed through open video device nodes.
Move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() to ensure that the entity isn't unregistered until the last reference to the video device is released.

Quoted 7 lines

Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those functions were designed for subdevs, to avoid a parent module from being removed while still accessible through board code. They're not currently needed for video device nodes, and will oops when a hotpluggable device is disconnected during streaming, as media_entity_put() called in v4l2_device_release() tries to access entity->parent->dev->driver which is set to NULL when the device is disconnected.

Thanks for the patch, Laurent!

Acked-by Sakari Ailus <[email protected]>

Signature · 2 lines

Sakari Ailus
[email protected]

Hans Verkuil

Acked-by on v1 1/1

2011-06-16 09:03 UTC #8 lore raw

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

In reply to Laurent Pinchart

Quoted 9 lines · On Wednesday, June 15, 2011 10:36:26 Laurent Pinchart wrote:

Entities associated with video device nodes are unregistered in video_unregister_device(). This destroys the entity even though it can still be accessed through open video device nodes.
Move the media_device_unregister_entity() call from video_unregister_device() to v4l2_device_release() to ensure that the entity isn't unregistered until the last reference to the video device is released.

Quoted 7 lines

Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those functions were designed for subdevs, to avoid a parent module from being removed while still accessible through board code. They're not currently needed for video device nodes, and will oops when a hotpluggable device is disconnected during streaming, as media_entity_put() called in v4l2_device_release() tries to access entity->parent->dev->driver which is set to NULL when the device is disconnected.

Looks good.

Acked-by Hans Verkuil <[email protected]>

Regards,

Hans

drivers/media/video/v4l2-dev.c static int v4l2_open(struct inode *inode, struct file

Quoted 24 lines

Signed-off-by Laurent Pinchart <[email protected]>

---
 drivers/media/video/v4l2-dev.c |   36 +++++++-----------------------------
 1 files changed, 7 insertions(+), 29 deletions(-)

drivers/media/video/v4l2-dev.c +6 -0

index 19d5ae2..5dc1fa1 100644
@@ -167,6 +167,12 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
+	    vdev->vfl_type != VFL_TYPE_SUBDEV)
+		media_device_unregister_entity(&vdev->entity);
+#endif
+
 	/* Release video_device and perform other
 	   cleanups as needed. */
 	vdev->release(vdev);

@@ -405,17 +411,6 @@ static int v4l2_open(struct inode *inode, struct file

*filp)

drivers/media/video/v4l2-dev.c static int v4l2_open(struct inode *inode, struct file

Quoted 18 lines

drivers/media/video/v4l2-dev.c +0 ~~-11~~

 	/* and increase the device refcount */
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
-		entity = media_entity_get(&vdev->entity);
-		if (!entity) {
-			ret = -EBUSY;
-			video_put(vdev);
-			return ret;
-		}
-	}
-#endif
 	if (vdev->fops->open) {
 		if (vdev->lock && mutex_lock_interruptible(vdev->lock)) {
 			ret = -ERESTARTSYS;

@@ -431,14 +426,8 @@ static int v4l2_open(struct inode *inode, struct file

*filp)

drivers/media/video/v4l2-dev.c static int v4l2_release(struct inode *inode, struct

Quoted 16 lines

drivers/media/video/v4l2-dev.c +1 -7

 err:
 	/* decrease the refcount in case of an error */
-	if (ret) {
-#if defined(CONFIG_MEDIA_CONTROLLER)
-		if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-		    vdev->vfl_type != VFL_TYPE_SUBDEV)
-			media_entity_put(entity);
-#endif
+	if (ret)
 		video_put(vdev);
-	}
 	return ret;
 }

@@ -455,11 +444,6 @@ static int v4l2_release(struct inode *inode, struct

file *filp)

Quoted 33 lines

drivers/media/video/v4l2-dev.c +0 ~~-11~~

 		if (vdev->lock)
 			mutex_unlock(vdev->lock);
 	}
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_entity_put(&vdev->entity);
-#endif
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	video_put(vdev);

@@ -754,12 +738,6 @@ void video_unregister_device(struct video_device *vdev)
 	if (!vdev || !video_is_registered(vdev))
 		return;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_device_unregister_entity(&vdev->entity);
-#endif
-
 	mutex_lock(&videodev_lock);
 	/* This must be in a critical section to prevent a race with v4l2_open.
 	 * Once this bit has been cleared video_get may never be called again.

Signature · 6 lines

1.7.3.4

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