linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: linux-media@vger.kernel.org
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
	Martin Kepplinger <martink@posteo.de>,
	Ricardo Ribalda <ribalda@kernel.org>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Bingbu Cao <bingbu.cao@intel.com>,
	"Paul J. Murphy" <paul.j.murphy@intel.com>,
	Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
	Tianshu Qiu <tian.shu.qiu@intel.com>,
	Jimmy Su <jimmy.su@intel.com>,
	Jason Chen <jason.z.chen@intel.com>,
	Arec Kao <arec.kao@intel.com>,
	Shunqian Zheng <zhengsq@rock-chips.com>,
	Jacopo Mondi <jacopo@jmondi.org>,
	Mikhail Rudenko <mike.rudenko@gmail.com>
Subject: [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements
Date: Mon, 18 Sep 2023 15:48:38 +0300	[thread overview]
Message-ID: <20230918124838.14210-1-laurent.pinchart@ideasonboard.com> (raw)

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

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

base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
Regards,

Laurent Pinchart


             reply	other threads:[~2023-09-18 12:50 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-18 12:48 Laurent Pinchart [this message]
2023-10-24  8:57 ` [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements Geert Uytterhoeven
2023-10-24 14:27   ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230918124838.14210-1-laurent.pinchart@ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=arec.kao@intel.com \
    --cc=bingbu.cao@intel.com \
    --cc=daniele.alessandrelli@intel.com \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=jacopo@jmondi.org \
    --cc=jason.z.chen@intel.com \
    --cc=jimmy.su@intel.com \
    --cc=linux-media@vger.kernel.org \
    --cc=martink@posteo.de \
    --cc=mike.rudenko@gmail.com \
    --cc=paul.j.murphy@intel.com \
    --cc=ribalda@kernel.org \
    --cc=sakari.ailus@iki.fi \
    --cc=tian.shu.qiu@intel.com \
    --cc=zhengsq@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).