| Message ID | 20220911171653.568932-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: further cleanups / unwanted code removal | expand |
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@linux.intel.com> (One minor question below) > 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(-) > > 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 >
Hi, On 9/12/22 13:30, Andy Shevchenko wrote: > 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@linux.intel.com> > > (One minor question below) > >> 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(-) >> >> 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? 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. Regards, Hans > >> + 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 >> >
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 --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);
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(-)