On Sun, Sep 11, 2022 at 07:16:41PM +0200, Hans de Goede wrote:
> 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.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
(One minor question below)
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../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(-)
>
> 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;
Wouldn't -EAGAIN match better in this case?
> + 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);
>
> --
> 2.37.3
>
--
With Best Regards,
Andy Shevchenko
