[10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop
diff mbox series

Message ID BN6PR04MB0660DB1C884EE9F9C7D94857A3AE0@BN6PR04MB0660.namprd04.prod.outlook.com
State Not Applicable
Headers show
Series
  • media: exynos4-is: Improve support for s5pv210 and parallel ports
Related show

Commit Message

Jonathan Bakker April 26, 2020, 2:26 a.m. UTC
media_pipeline_stop can be called from both release and streamoff,
so make sure they're both protected under the streaming flag and
not just one of them.

This probably became noticeable after commit 2a2599c66368 ("[media] media:
entity: Catch unbalanced media_pipeline_stop calls") was merged as it
added a WARN.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-capture.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Tomasz Figa July 7, 2020, 6:44 p.m. UTC | #1
Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
> media_pipeline_stop can be called from both release and streamoff,
> so make sure they're both protected under the streaming flag and
> not just one of them.

First of all, thanks for the patch.

Shouldn't it be that release calls streamoff, so that only streamoff
is supposed to have the call to media_pipeline_stop()?

Best regards,
Tomasz
Jonathan Bakker July 11, 2020, 6:17 p.m. UTC | #2
Hi Tomasz,

On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
> Hi Jonathan,
> 
> On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
>> media_pipeline_stop can be called from both release and streamoff,
>> so make sure they're both protected under the streaming flag and
>> not just one of them.
> 
> First of all, thanks for the patch.
> 
> Shouldn't it be that release calls streamoff, so that only streamoff
> is supposed to have the call to media_pipeline_stop()?
> 

I can't say that I understand the whole media subsystem enough to know :)
Since media_pipeline_start is called in streamon, it makes sense that streamoff
should have the media_pipeline_stop call.  However, even after removing the call
in fimc_capture_release I'm still getting a backtrace such as

[   73.843117] ------------[ cut here ]------------
[   73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
[   73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
[   73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
[   73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
[   73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
[   73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
[   73.844546] 3fa0:                   0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
[   73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
[   73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
[   73.844642] ---[ end trace e6a4a8b2f20addd4 ]---

The command I'm using for testing is

v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1

Since I noticed that the streaming flag was being checked fimc_capture_release
but not in fimc_cap_streamoff, I assumed that it was simply a missed check.  Comparing
with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
callback.

I'm willing to test various options

> Best regards,
> Tomasz
> 

Thanks,
Jonathan

Patch
diff mbox series

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 95d4a667bffb..d3ef1268da07 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1232,8 +1232,11 @@  static int fimc_cap_streamoff(struct file *file, void *priv,
 	if (ret < 0)
 		return ret;
 
-	media_pipeline_stop(&vc->ve.vdev.entity);
-	vc->streaming = false;
+	if (vc->streaming) {
+		media_pipeline_stop(&vc->ve.vdev.entity);
+		vc->streaming = false;
+	}
+
 	return 0;
 }