diff mbox series

[05/17] media: atomisp: Add atomisp_pipe_check() helper

Message ID 20220911171653.568932-6-hdegoede@redhat.com
State Accepted
Commit f315c1acba84fe17cd92a05e6c1c1bf26ee6bd43
Headers show
Series media: atomisp: further cleanups / unwanted code removal | expand

Commit Message

Hans de Goede Sept. 11, 2022, 5:16 p.m. UTC
Several of the ioctl handlers all do the same checks
(isp->fatal_error and asd->streaming errors) add
an atomisp_pipe_check() helper for this.

Note this changes the vidioc_s_fmt_vid_cap and vidioc_s_input handlers
to now reject calls made while asd->streaming==STOPPING. This fixes
a possible race where one thread can make this ioctls while
vidioc_streamoff is running from another thread and it has
temporarily released isp->mutex to kill the watchdog timers / work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   |  9 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 89 +++++++++----------
 .../staging/media/atomisp/pci/atomisp_ioctl.h |  2 +
 3 files changed, 48 insertions(+), 52 deletions(-)

Comments

Andy Shevchenko Sept. 21, 2022, 11:55 a.m. UTC | #1
On Wed, Sep 21, 2022 at 12:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/12/22 13:30, Andy Shevchenko wrote:
> > On Sun, Sep 11, 2022 at 07:16:41PM +0200, Hans de Goede wrote:

...

> >> +    case ATOMISP_DEVICE_STREAMING_STOPPING:
> >> +            dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
> >> +            return -EBUSY;
> >
> > Wouldn't -EAGAIN match better in this case?
>
> The original checks this replaces used -EIO (which seems like a poor
> choice) resp. -EBUSY (in the streamon callback) so I decided to
> keep the -EBUSY here.
>
> Also AFAIK -EAGAIN will make the C-library retry the syscal itself
> in some cases ? (not sure if this applies to ioctls though).
>
> This is not what we want, this scenario can only be hit when an app:
> 1. Uses both the preview and the actual capture /dev/video# nodes
>    at the same time (this is allows)
> 2. Then stops the stream at 1 of them, this transitions the state
>    to STOPPING
> 3. Then does some ioctl other then streamoff on the other /dev/video#
>
> Basically when using more then 1 /dev/video# node then the app must
> stop all of them when stopping things. The driver enforces this
> by rejecting all calls other the streamoff until all /dev/video#
> node streans are off.
>
> This means that simply trying again will result in the same error,
> so -EBUSY seems like the best error for this.

Thanks for the explanation, now it's clear to me.
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 087078900415..7945852ecd13 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -5549,16 +5549,13 @@  int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct v4l2_subdev_fh fh;
 	int ret;
 
-	lockdep_assert_held(&isp->mutex);
+	ret = atomisp_pipe_check(pipe, true);
+	if (ret)
+		return ret;
 
 	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
 		return -EINVAL;
 
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
-		dev_warn(isp->dev, "ISP does not support set format while at streaming!\n");
-		return -EBUSY;
-	}
-
 	dev_dbg(isp->dev,
 		"setting resolution %ux%u on pad %u for asd%d, bytesperline %u\n",
 		f->fmt.pix.width, f->fmt.pix.height, source_pad,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 9c7022be3a06..9b50f637c46a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -535,6 +535,32 @@  atomisp_get_format_bridge_from_mbus(u32 mbus_code)
 	return NULL;
 }
 
+int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool settings_change)
+{
+	lockdep_assert_held(&pipe->isp->mutex);
+
+	if (pipe->isp->isp_fatal_error)
+		return -EIO;
+
+	switch (pipe->asd->streaming) {
+	case ATOMISP_DEVICE_STREAMING_DISABLED:
+		break;
+	case ATOMISP_DEVICE_STREAMING_ENABLED:
+		if (settings_change) {
+			dev_err(pipe->isp->dev, "Set fmt/input IOCTL while streaming\n");
+			return -EBUSY;
+		}
+		break;
+	case ATOMISP_DEVICE_STREAMING_STOPPING:
+		dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
+		return -EBUSY;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * v4l2 ioctls
  * return ISP capabilities
@@ -646,12 +672,18 @@  static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_device *isp = video_get_drvdata(vdev);
-	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
+	struct atomisp_sub_device *asd = pipe->asd;
 	struct v4l2_subdev *camera = NULL;
 	struct v4l2_subdev *motor;
 	int ret;
 
 	mutex_lock(&isp->mutex);
+
+	ret = atomisp_pipe_check(pipe, true);
+	if (ret)
+		goto error;
+
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
 		ret = -EINVAL;
@@ -678,13 +710,6 @@  static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 		goto error;
 	}
 
-	if (atomisp_subdev_streaming_count(asd)) {
-		dev_err(isp->dev,
-			"ISP is still streaming, stop first\n");
-		ret = -EINVAL;
-		goto error;
-	}
-
 	/* power off the current owned sensor, as it is not used this time */
 	if (isp->inputs[asd->input_curr].asd == asd &&
 	    asd->input_curr != input) {
@@ -976,11 +1001,6 @@  static int atomisp_s_fmt_cap(struct file *file, void *fh,
 	int ret;
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		mutex_unlock(&isp->mutex);
-		return ret;
-	}
 	ret = atomisp_set_fmt(vdev, f);
 	mutex_unlock(&isp->mutex);
 	return ret;
@@ -1236,20 +1256,13 @@  static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct ia_css_frame *handle = NULL;
 	u32 length;
 	u32 pgnr;
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		goto error;
-	}
 
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
-			__func__);
-		ret = -EIO;
+	ret = atomisp_pipe_check(pipe, false);
+	if (ret)
 		goto error;
-	}
 
 	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
 	    !pipe->capq.bufs[buf->index]) {
@@ -1418,23 +1431,13 @@  static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&isp->mutex);
-
-	if (isp->isp_fatal_error) {
-		mutex_unlock(&isp->mutex);
-		return -EIO;
-	}
-
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		mutex_unlock(&isp->mutex);
-		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
-			__func__);
-		return -EIO;
-	}
-
+	ret = atomisp_pipe_check(pipe, false);
 	mutex_unlock(&isp->mutex);
+	if (ret)
+		return ret;
 
 	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
 	if (ret) {
@@ -1668,8 +1671,8 @@  static int atomisp_streamon(struct file *file, void *fh,
 	enum ia_css_pipe_id css_pipe_id;
 	unsigned int sensor_start_stream;
 	unsigned int wdt_duration = ATOMISP_ISP_TIMEOUT_DURATION;
-	int ret = 0;
 	unsigned long irqflags;
+	int ret;
 
 	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
@@ -1680,15 +1683,9 @@  static int atomisp_streamon(struct file *file, void *fh,
 	}
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		goto out;
-	}
-
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		ret = -EBUSY;
+	ret = atomisp_pipe_check(pipe, false);
+	if (ret)
 		goto out;
-	}
 
 	if (pipe->capq.streaming)
 		goto out;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
index 382b78275240..61a6148a6ad5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
@@ -34,6 +34,8 @@  atomisp_format_bridge *atomisp_get_format_bridge(unsigned int pixelformat);
 const struct
 atomisp_format_bridge *atomisp_get_format_bridge_from_mbus(u32 mbus_code);
 
+int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool streaming_ok);
+
 int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 				uint16_t stream_id);