diff mbox series

[v2] media: v4l2-subdev: Document and enforce .s_stream() requirements

Message ID 20230918124838.14210-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 009905ec50433259c05f474251000b040098564e
Headers show
Series [v2] media: v4l2-subdev: Document and enforce .s_stream() requirements | expand

Commit Message

Laurent Pinchart Sept. 18, 2023, 12:48 p.m. UTC 
The subdev .s_stream() operation must not be called to start an already
started subdev, or stop an already stopped one. This requirement has
never been formally documented. Fix it, and catch possible offenders
with a WARN_ON() in the call_s_stream() wrapper.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Add WARN_ON() in call_s_stream()
- Fix typo and language in documentation
---
I'm resending this patch individually to avoid spamming the list with
the 56 other patches included in v1. You can find the original series at
https://lore.kernel.org/linux-media/20230914181704.4811-1-laurent.pinchart@ideasonboard.com
---
 drivers/media/v4l2-core/v4l2-subdev.c | 17 ++++++++++++++++-
 include/media/v4l2-subdev.h           |  4 +++-
 2 files changed, 19 insertions(+), 2 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d

Comments

Geert Uytterhoeven Oct. 24, 2023, 8:57 a.m. UTC | #1 
Hi Laurent,

On Mon, 18 Sep 2023, Laurent Pinchart wrote:
> The subdev .s_stream() operation must not be called to start an already
> started subdev, or stop an already stopped one. This requirement has
> never been formally documented. Fix it, and catch possible offenders
> with a WARN_ON() in the call_s_stream() wrapper.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Add WARN_ON() in call_s_stream()
> - Fix typo and language in documentation

Thanks for your patch, which is now commit 009905ec50433259 ("media:
v4l2-subdev: Document and enforce .s_stream() requirements")
in linux-next/master media/master.

This patch causes the below warning during s2idle/s2ram on Salvator-XS
(R-Car H3 ES2.0) and White-Hawk (R-Car V4H).  Koelsch (R-Car M2-W) is not
affected, as its DU does not use the VSP.

     Filesystems sync: 0.014 seconds
     Freezing user space processes
     Freezing user space processes completed (elapsed 0.006 seconds)
     OOM killer disabled.
     Freezing remaining freezable tasks
     Freezing remaining freezable tasks completed (elapsed 0.002 seconds)
     ------------[ cut here ]------------
     WARNING: CPU: 2 PID: 962 at drivers/media/v4l2-core/v4l2-subdev.c:371 call_s_stream+0xd4/0xf0
     CPU: 2 PID: 962 Comm: s2idle Not tainted 6.6.0-rc3-arm64-renesas-00068-g009905ec5043 #2302
     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
     pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
     pc : call_s_stream+0xd4/0xf0
     lr : vsp1_pipeline_stop+0x10c/0x2a4
     sp : ffff800083f635f0
     x29: ffff800083f635f0 x28: 0000000000000000 x27: 0000000000000001
     x26: 0000000000000000 x25: ffff0004c3375b08 x24: ffff0004c3375880
     x23: 0000000000000288 x22: ffff0004c3375b30 x21: 0000000000000000
     x20: ffff0004c3370880 x19: ffff0004c33750f0 x18: 0000000000000001
     x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000028
     x14: 0000000000000000 x13: 0000000000000003 x12: 0000000000000000
     x11: 0000000000000000 x10: ffff800082063b78 x9 : ffff800081a5abb8
     x8 : 00000000e50359b8 x7 : 00000000000000ac x6 : 0000000000002b10
     x5 : 0000000000000000 x4 : ffff8000838f8000 x3 : ffff800080932760
     x2 : ffff80008096671c x1 : 0000000000000000 x0 : 0000000000000001
     Call trace:
      call_s_stream+0xd4/0xf0
      vsp1_pipeline_stop+0x10c/0x2a4
      vsp1_du_setup_lif+0x324/0x468
      rcar_du_vsp_disable+0x1c/0x24
      rcar_du_crtc_atomic_disable+0x2f8/0x438
      disable_outputs+0x22c/0x338
      drm_atomic_helper_commit_modeset_disables+0x18/0x44
      rcar_du_atomic_commit_tail+0x90/0xd8
      commit_tail+0x9c/0x178
      drm_atomic_helper_commit+0x18c/0x1a0
      drm_atomic_commit+0xa4/0xd8
      drm_atomic_helper_disable_all+0x1ec/0x200
      drm_atomic_helper_suspend+0xa0/0x208
      drm_mode_config_helper_suspend+0x2c/0x70
      rcar_du_pm_suspend+0x14/0x1c
      platform_pm_suspend+0x28/0x64
      dpm_run_callback+0x34/0x90
      __device_suspend+0x10c/0x3c4
      dpm_suspend+0x140/0x250
      dpm_suspend_start+0x78/0x90
      suspend_devices_and_enter+0x124/0x570
      pm_suspend+0x1ec/0x310
      state_store+0x88/0x124
      kobj_attr_store+0x14/0x24
      sysfs_kf_write+0x48/0x6c
      kernfs_fop_write_iter+0x118/0x1a8
      vfs_write+0x208/0x30c
      ksys_write+0x64/0xec
      __arm64_sys_write+0x18/0x20
      invoke_syscall+0x44/0x108
      el0_svc_common.constprop.0+0x3c/0xdc
      do_el0_svc+0x1c/0x24
      el0_svc+0x40/0xd4
      el0t_64_sync_handler+0x134/0x150
      el0t_64_sync+0x14c/0x150
     irq event stamp: 19852
     hardirqs last  enabled at (19851): [<ffff800080dc3234>] _raw_spin_unlock_irqrestore+0x6c/0x70
     hardirqs last disabled at (19852): [<ffff800080db3f9c>] el1_dbg+0x20/0x80
     softirqs last  enabled at (17806): [<ffff800080010618>] __do_softirq+0x430/0x4e0
     softirqs last disabled at (17799): [<ffff8000800151e0>] ____do_softirq+0xc/0x14
     ---[ end trace 0000000000000000 ]---

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds
Laurent Pinchart Oct. 24, 2023, 2:27 p.m. UTC | #2 
Hi Geert,

On Tue, Oct 24, 2023 at 10:57:26AM +0200, Geert Uytterhoeven wrote:
>  	Hi Laurent,
> 
> On Mon, 18 Sep 2023, Laurent Pinchart wrote:
> > The subdev .s_stream() operation must not be called to start an already
> > started subdev, or stop an already stopped one. This requirement has
> > never been formally documented. Fix it, and catch possible offenders
> > with a WARN_ON() in the call_s_stream() wrapper.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Add WARN_ON() in call_s_stream()
> > - Fix typo and language in documentation
> 
> Thanks for your patch, which is now commit 009905ec50433259 ("media:
> v4l2-subdev: Document and enforce .s_stream() requirements")
> in linux-next/master media/master.
> 
> This patch causes the below warning during s2idle/s2ram on Salvator-XS
> (R-Car H3 ES2.0) and White-Hawk (R-Car V4H).  Koelsch (R-Car M2-W) is not
> affected, as its DU does not use the VSP.

Oops :-S

https://lore.kernel.org/linux-media/20231024142522.29658-1-laurent.pinchart+renesas@ideasonboard.com/T/#u
should fix it.

>      Filesystems sync: 0.014 seconds
>      Freezing user space processes
>      Freezing user space processes completed (elapsed 0.006 seconds)
>      OOM killer disabled.
>      Freezing remaining freezable tasks
>      Freezing remaining freezable tasks completed (elapsed 0.002 seconds)
>      ------------[ cut here ]------------
>      WARNING: CPU: 2 PID: 962 at drivers/media/v4l2-core/v4l2-subdev.c:371 call_s_stream+0xd4/0xf0
>      CPU: 2 PID: 962 Comm: s2idle Not tainted 6.6.0-rc3-arm64-renesas-00068-g009905ec5043 #2302
>      Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
>      pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>      pc : call_s_stream+0xd4/0xf0
>      lr : vsp1_pipeline_stop+0x10c/0x2a4
>      sp : ffff800083f635f0
>      x29: ffff800083f635f0 x28: 0000000000000000 x27: 0000000000000001
>      x26: 0000000000000000 x25: ffff0004c3375b08 x24: ffff0004c3375880
>      x23: 0000000000000288 x22: ffff0004c3375b30 x21: 0000000000000000
>      x20: ffff0004c3370880 x19: ffff0004c33750f0 x18: 0000000000000001
>      x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000028
>      x14: 0000000000000000 x13: 0000000000000003 x12: 0000000000000000
>      x11: 0000000000000000 x10: ffff800082063b78 x9 : ffff800081a5abb8
>      x8 : 00000000e50359b8 x7 : 00000000000000ac x6 : 0000000000002b10
>      x5 : 0000000000000000 x4 : ffff8000838f8000 x3 : ffff800080932760
>      x2 : ffff80008096671c x1 : 0000000000000000 x0 : 0000000000000001
>      Call trace:
>       call_s_stream+0xd4/0xf0
>       vsp1_pipeline_stop+0x10c/0x2a4
>       vsp1_du_setup_lif+0x324/0x468
>       rcar_du_vsp_disable+0x1c/0x24
>       rcar_du_crtc_atomic_disable+0x2f8/0x438
>       disable_outputs+0x22c/0x338
>       drm_atomic_helper_commit_modeset_disables+0x18/0x44
>       rcar_du_atomic_commit_tail+0x90/0xd8
>       commit_tail+0x9c/0x178
>       drm_atomic_helper_commit+0x18c/0x1a0
>       drm_atomic_commit+0xa4/0xd8
>       drm_atomic_helper_disable_all+0x1ec/0x200
>       drm_atomic_helper_suspend+0xa0/0x208
>       drm_mode_config_helper_suspend+0x2c/0x70
>       rcar_du_pm_suspend+0x14/0x1c
>       platform_pm_suspend+0x28/0x64
>       dpm_run_callback+0x34/0x90
>       __device_suspend+0x10c/0x3c4
>       dpm_suspend+0x140/0x250
>       dpm_suspend_start+0x78/0x90
>       suspend_devices_and_enter+0x124/0x570
>       pm_suspend+0x1ec/0x310
>       state_store+0x88/0x124
>       kobj_attr_store+0x14/0x24
>       sysfs_kf_write+0x48/0x6c
>       kernfs_fop_write_iter+0x118/0x1a8
>       vfs_write+0x208/0x30c
>       ksys_write+0x64/0xec
>       __arm64_sys_write+0x18/0x20
>       invoke_syscall+0x44/0x108
>       el0_svc_common.constprop.0+0x3c/0xdc
>       do_el0_svc+0x1c/0x24
>       el0_svc+0x40/0xd4
>       el0t_64_sync_handler+0x134/0x150
>       el0t_64_sync+0x14c/0x150
>      irq event stamp: 19852
>      hardirqs last  enabled at (19851): [<ffff800080dc3234>] _raw_spin_unlock_irqrestore+0x6c/0x70
>      hardirqs last disabled at (19852): [<ffff800080db3f9c>] el1_dbg+0x20/0x80
>      softirqs last  enabled at (17806): [<ffff800080010618>] __do_softirq+0x430/0x4e0
>      softirqs last disabled at (17799): [<ffff8000800151e0>] ____do_softirq+0xc/0x14
>      ---[ end trace 0000000000000000 ]---
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b92348ad61f6..32b7d9cd43e6 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -359,6 +359,18 @@  static int call_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	int ret;
 
+	/*
+	 * The .s_stream() operation must never be called to start or stop an
+	 * already started or stopped subdev. Catch offenders but don't return
+	 * an error yet to avoid regressions.
+	 *
+	 * As .s_stream() is mutually exclusive with the .enable_streams() and
+	 * .disable_streams() operation, we can use the enabled_streams field
+	 * to store the subdev streaming state.
+	 */
+	if (WARN_ON(!!sd->enabled_streams == !!enable))
+		return 0;
+
 #if IS_REACHABLE(CONFIG_LEDS_CLASS)
 	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
 		if (enable)
@@ -372,9 +384,12 @@  static int call_s_stream(struct v4l2_subdev *sd, int enable)
 
 	if (!enable && ret < 0) {
 		dev_warn(sd->dev, "disabling streaming failed (%d)\n", ret);
-		return 0;
+		ret = 0;
 	}
 
+	if (!ret)
+		sd->enabled_streams = enable ? BIT(0) : 0;
+
 	return ret;
 }
 
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d9fca929c10b..ab2a7ef61d42 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -446,7 +446,9 @@  enum v4l2_subdev_pre_streamon_flags {
  * @s_stream: start (enabled == 1) or stop (enabled == 0) streaming on the
  *	sub-device. Failure on stop will remove any resources acquired in
  *	streaming start, while the error code is still returned by the driver.
- *	Also see call_s_stream wrapper in v4l2-subdev.c.
+ *	The caller shall track the subdev state, and shall not start or stop an
+ *	already started or stopped subdev. Also see call_s_stream wrapper in
+ *	v4l2-subdev.c.
  *
  * @g_pixelaspect: callback to return the pixelaspect ratio.
  *