diff mbox series

[v4,2/5] media: vimc: Fix gpf in rmmod path when stream is active

Message ID 2421a0ea4bec8c5084121ecdbc9213c331239f3d.1567822793.git.skhan@linuxfoundation.org (mailing list archive)
State New, archived
Headers show
Series Collapse vimc into single monolithic driver | expand

Commit Message

Shuah Khan Sept. 7, 2019, 2:42 a.m. UTC 
If vimc module is removed while streaming is in progress, sensor subdev
unregister runs into general protection fault when it tries to unregister
media entities. This is a common subdev problem related to releasing
pads from v4l2_device_unregister_subdev() before calling unregister.
Unregister references pads during unregistering subdev.

The sd release handler is the right place for releasing all sd resources
including pads. The release handlers currently release all resources
except the pads.

Fix v4l2_device_unregister_subdev() not release pads and release pads
from the sd_int_op release handlers.

kernel: [ 4136.715839] general protection fault: 0000 [#1] SMP PTI
kernel: [ 4136.715847] CPU: 2 PID: 1972 Comm: bash Not tainted 5.3.0-rc2+ #4
kernel: [ 4136.715850] Hardware name: Dell Inc. OptiPlex 790/0HY9JP, BIOS A18 09/24/2013
kernel: [ 4136.715858] RIP: 0010:media_gobj_destroy.part.16+0x1f/0x60
kernel: [ 4136.715863] Code: ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 fe 48 89 e5 53 48 89 fb 48 c7 c7 00 7f cf b0 e8 24 fa ff ff 48 8b 03 <48> 83 80 a0 00 00 00 01 48 8b 43 18 48 8b 53 10 48 89 42 08 48 89
kernel: [ 4136.715866] RSP: 0018:ffff9b2248fe3cb0 EFLAGS: 00010246
kernel: [ 4136.715870] RAX: bcf2bfbfa0d63c2f RBX: ffff88c3eb37e9c0 RCX: 00000000802a0018
kernel: [ 4136.715873] RDX: ffff88c3e4f6a078 RSI: ffff88c3eb37e9c0 RDI: ffffffffb0cf7f00
kernel: [ 4136.715876] RBP: ffff9b2248fe3cb8 R08: 0000000001000002 R09: ffffffffb0492b00
kernel: [ 4136.715879] R10: ffff9b2248fe3c28 R11: 0000000000000001 R12: 0000000000000038
kernel: [ 4136.715881] R13: ffffffffc09a1628 R14: ffff88c3e4f6a028 R15: fffffffffffffff2
kernel: [ 4136.715885] FS:  00007f8389647740(0000) GS:ffff88c465500000(0000) knlGS:0000000000000000
kernel: [ 4136.715888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: [ 4136.715891] CR2: 000055d008f80fd8 CR3: 00000001996ec005 CR4: 00000000000606e0
kernel: [ 4136.715894] Call Trace:
kernel: [ 4136.715903]  media_gobj_destroy+0x14/0x20
kernel: [ 4136.715908]  __media_device_unregister_entity+0xb3/0xe0
kernel: [ 4136.715915]  media_device_unregister_entity+0x30/0x40
kernel: [ 4136.715920]  v4l2_device_unregister_subdev+0xa8/0xe0
kernel: [ 4136.715928]  vimc_ent_sd_unregister+0x1e/0x30 [vimc]
kernel: [ 4136.715933]  vimc_sen_rm+0x16/0x20 [vimc]
kernel: [ 4136.715938]  vimc_remove+0x3e/0xa0 [vimc]
kernel: [ 4136.715945]  platform_drv_remove+0x25/0x50
kernel: [ 4136.715951]  device_release_driver_internal+0xe0/0x1b0
kernel: [ 4136.715956]  device_driver_detach+0x14/0x20
kernel: [ 4136.715960]  unbind_store+0xd1/0x130
kernel: [ 4136.715965]  drv_attr_store+0x27/0x40
kernel: [ 4136.715971]  sysfs_kf_write+0x48/0x60
kernel: [ 4136.715976]  kernfs_fop_write+0x128/0x1b0
kernel: [ 4136.715982]  __vfs_write+0x1b/0x40
kernel: [ 4136.715987]  vfs_write+0xc3/0x1d0
kernel: [ 4136.715993]  ksys_write+0xaa/0xe0
kernel: [ 4136.715999]  __x64_sys_write+0x1a/0x20
kernel: [ 4136.716005]  do_syscall_64+0x5a/0x130
kernel: [ 4136.716010]  entry_SYSCALL_64_after_hwframe+0x4
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/media/platform/vimc/vimc-common.c  | 3 +--
 drivers/media/platform/vimc/vimc-debayer.c | 1 +
 drivers/media/platform/vimc/vimc-scaler.c  | 1 +
 drivers/media/platform/vimc/vimc-sensor.c  | 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

Comments

Helen Mae Koike Fornazier Sept. 15, 2019, 7:25 p.m. UTC | #1 
On 9/6/19 11:42 PM, Shuah Khan wrote:
> If vimc module is removed while streaming is in progress, sensor subdev
> unregister runs into general protection fault when it tries to unregister
> media entities. This is a common subdev problem related to releasing
> pads from v4l2_device_unregister_subdev() before calling unregister.
> Unregister references pads during unregistering subdev.
> 
> The sd release handler is the right place for releasing all sd resources
> including pads. The release handlers currently release all resources
> except the pads.
> 
> Fix v4l2_device_unregister_subdev() not release pads and release pads
> from the sd_int_op release handlers.
> 
> kernel: [ 4136.715839] general protection fault: 0000 [#1] SMP PTI
> kernel: [ 4136.715847] CPU: 2 PID: 1972 Comm: bash Not tainted 5.3.0-rc2+ #4
> kernel: [ 4136.715850] Hardware name: Dell Inc. OptiPlex 790/0HY9JP, BIOS A18 09/24/2013
> kernel: [ 4136.715858] RIP: 0010:media_gobj_destroy.part.16+0x1f/0x60
> kernel: [ 4136.715863] Code: ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 fe 48 89 e5 53 48 89 fb 48 c7 c7 00 7f cf b0 e8 24 fa ff ff 48 8b 03 <48> 83 80 a0 00 00 00 01 48 8b 43 18 48 8b 53 10 48 89 42 08 48 89
> kernel: [ 4136.715866] RSP: 0018:ffff9b2248fe3cb0 EFLAGS: 00010246
> kernel: [ 4136.715870] RAX: bcf2bfbfa0d63c2f RBX: ffff88c3eb37e9c0 RCX: 00000000802a0018
> kernel: [ 4136.715873] RDX: ffff88c3e4f6a078 RSI: ffff88c3eb37e9c0 RDI: ffffffffb0cf7f00
> kernel: [ 4136.715876] RBP: ffff9b2248fe3cb8 R08: 0000000001000002 R09: ffffffffb0492b00
> kernel: [ 4136.715879] R10: ffff9b2248fe3c28 R11: 0000000000000001 R12: 0000000000000038
> kernel: [ 4136.715881] R13: ffffffffc09a1628 R14: ffff88c3e4f6a028 R15: fffffffffffffff2
> kernel: [ 4136.715885] FS:  00007f8389647740(0000) GS:ffff88c465500000(0000) knlGS:0000000000000000
> kernel: [ 4136.715888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: [ 4136.715891] CR2: 000055d008f80fd8 CR3: 00000001996ec005 CR4: 00000000000606e0
> kernel: [ 4136.715894] Call Trace:
> kernel: [ 4136.715903]  media_gobj_destroy+0x14/0x20
> kernel: [ 4136.715908]  __media_device_unregister_entity+0xb3/0xe0
> kernel: [ 4136.715915]  media_device_unregister_entity+0x30/0x40
> kernel: [ 4136.715920]  v4l2_device_unregister_subdev+0xa8/0xe0
> kernel: [ 4136.715928]  vimc_ent_sd_unregister+0x1e/0x30 [vimc]
> kernel: [ 4136.715933]  vimc_sen_rm+0x16/0x20 [vimc]
> kernel: [ 4136.715938]  vimc_remove+0x3e/0xa0 [vimc]
> kernel: [ 4136.715945]  platform_drv_remove+0x25/0x50
> kernel: [ 4136.715951]  device_release_driver_internal+0xe0/0x1b0
> kernel: [ 4136.715956]  device_driver_detach+0x14/0x20
> kernel: [ 4136.715960]  unbind_store+0xd1/0x130
> kernel: [ 4136.715965]  drv_attr_store+0x27/0x40
> kernel: [ 4136.715971]  sysfs_kf_write+0x48/0x60
> kernel: [ 4136.715976]  kernfs_fop_write+0x128/0x1b0
> kernel: [ 4136.715982]  __vfs_write+0x1b/0x40
> kernel: [ 4136.715987]  vfs_write+0xc3/0x1d0
> kernel: [ 4136.715993]  ksys_write+0xaa/0xe0
> kernel: [ 4136.715999]  __x64_sys_write+0x1a/0x20
> kernel: [ 4136.716005]  do_syscall_64+0x5a/0x130
> kernel: [ 4136.716010]  entry_SYSCALL_64_after_hwframe+0x4
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
>  drivers/media/platform/vimc/vimc-common.c  | 3 +--
>  drivers/media/platform/vimc/vimc-debayer.c | 1 +
>  drivers/media/platform/vimc/vimc-scaler.c  | 1 +
>  drivers/media/platform/vimc/vimc-sensor.c  | 1 +
>  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index 7e1ae0b12f1e..a3120f4f7a90 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -375,7 +375,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  {
>  	int ret;
>  
> -	/* Allocate the pads */
> +	/* Allocate the pads. Should be released from the sd_int_op release */
>  	ved->pads = vimc_pads_init(num_pads, pads_flag);
>  	if (IS_ERR(ved->pads))
>  		return PTR_ERR(ved->pads);
> @@ -424,7 +424,6 @@ EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
>  void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
>  {
>  	media_entity_cleanup(ved->ent);
> -	vimc_pads_cleanup(ved->pads);
>  	v4l2_device_unregister_subdev(sd);
>  }
>  EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 00850f2501ad..b38b55f51a24 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -482,6 +482,7 @@ static void vimc_deb_release(struct v4l2_subdev *sd)
>  	struct vimc_deb_device *vdeb =
>  				container_of(sd, struct vimc_deb_device, sd);
>  
> +	vimc_pads_cleanup(vdeb->ved.pads);
>  	kfree(vdeb);
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 28a947950a0b..05db5070e268 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -338,6 +338,7 @@ static void vimc_sca_release(struct v4l2_subdev *sd)
>  	struct vimc_sca_device *vsca =
>  				container_of(sd, struct vimc_sca_device, sd);
>  
> +	vimc_pads_cleanup(vsca->ved.pads);
>  	kfree(vsca);
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 1f15637ca8bb..46dc6a535abe 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -291,6 +291,7 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
>  
>  	v4l2_ctrl_handler_free(&vsen->hdl);
>  	tpg_free(&vsen->tpg);
> +	vimc_pads_cleanup(vsen->ved.pads);
>  	kfree(vsen);
>  }
>  
>
diff mbox series

Patch

diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index 7e1ae0b12f1e..a3120f4f7a90 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -375,7 +375,7 @@  int vimc_ent_sd_register(struct vimc_ent_device *ved,
 {
 	int ret;
 
-	/* Allocate the pads */
+	/* Allocate the pads. Should be released from the sd_int_op release */
 	ved->pads = vimc_pads_init(num_pads, pads_flag);
 	if (IS_ERR(ved->pads))
 		return PTR_ERR(ved->pads);
@@ -424,7 +424,6 @@  EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
 void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
 {
 	media_entity_cleanup(ved->ent);
-	vimc_pads_cleanup(ved->pads);
 	v4l2_device_unregister_subdev(sd);
 }
 EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 00850f2501ad..b38b55f51a24 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -482,6 +482,7 @@  static void vimc_deb_release(struct v4l2_subdev *sd)
 	struct vimc_deb_device *vdeb =
 				container_of(sd, struct vimc_deb_device, sd);
 
+	vimc_pads_cleanup(vdeb->ved.pads);
 	kfree(vdeb);
 }
 
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 28a947950a0b..05db5070e268 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -338,6 +338,7 @@  static void vimc_sca_release(struct v4l2_subdev *sd)
 	struct vimc_sca_device *vsca =
 				container_of(sd, struct vimc_sca_device, sd);
 
+	vimc_pads_cleanup(vsca->ved.pads);
 	kfree(vsca);
 }
 
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 1f15637ca8bb..46dc6a535abe 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -291,6 +291,7 @@  static void vimc_sen_release(struct v4l2_subdev *sd)
 
 	v4l2_ctrl_handler_free(&vsen->hdl);
 	tpg_free(&vsen->tpg);
+	vimc_pads_cleanup(vsen->ved.pads);
 	kfree(vsen);
 }