On Sun, Sep 11, 2022 at 07:16:39PM +0200, Hans de Goede wrote:
> For reading / writing the asd->streaming enum the following rules
> should be followed:
>
> 1. Writers of streaming must hold both isp->mutex and isp->lock.
> 2. Readers of streaming need to hold only one of the two locks.
>
> Not all writers where properly taking both locks this fixes this.
>
> In the case of the readers, many readers depend on their caller
> to hold isp->mutex, add asserts for this
>
> And in the case of atomisp_css_get_dis_stat() it is called with
> isp->mutex held, so there is no need to take the spinlock just
> for reading the streaming value.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Although description of the 'streaming' doesn't clarify which one should be
used on which circumstances. I assume it's only for sleeping / non-sleeping
context? If there is an IRQ relation, shouldn't we always use spinlock for
readers?
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../staging/media/atomisp/pci/atomisp_cmd.c | 32 +++++++++++++++++--
> .../media/atomisp/pci/atomisp_compat_css20.c | 10 +++---
> .../staging/media/atomisp/pci/atomisp_fops.c | 3 ++
> .../media/atomisp/pci/atomisp_internal.h | 2 +-
> .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +++
> .../media/atomisp/pci/atomisp_subdev.c | 8 ++++-
> .../media/atomisp/pci/atomisp_subdev.h | 6 +++-
> 7 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 97ef02e4e7a6..c7f825e38921 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -899,6 +899,8 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
> struct v4l2_control ctrl;
> bool reset_wdt_timer = false;
>
> + lockdep_assert_held(&isp->mutex);
> +
> if (
> buf_type != IA_CSS_BUFFER_TYPE_METADATA &&
> buf_type != IA_CSS_BUFFER_TYPE_3A_STATISTICS &&
> @@ -1298,6 +1300,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
> bool stream_restart[MAX_STREAM_NUM] = {0};
> bool depth_mode = false;
> int i, ret, depth_cnt = 0;
> + unsigned long flags;
> +
> + lockdep_assert_held(&isp->mutex);
>
> atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
>
> @@ -1320,7 +1325,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
>
> stream_restart[asd->index] = true;
>
> + spin_lock_irqsave(&isp->lock, flags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
> + spin_unlock_irqrestore(&isp->lock, flags);
>
> /* stream off sensor */
> ret = v4l2_subdev_call(
> @@ -1335,7 +1342,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
> css_pipe_id = atomisp_get_css_pipe_id(asd);
> atomisp_css_stop(asd, css_pipe_id, true);
>
> + spin_lock_irqsave(&isp->lock, flags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
> + spin_unlock_irqrestore(&isp->lock, flags);
>
> asd->preview_exp_id = 1;
> asd->postview_exp_id = 1;
> @@ -1376,11 +1385,14 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
> IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
>
> css_pipe_id = atomisp_get_css_pipe_id(asd);
> - if (atomisp_css_start(asd, css_pipe_id, true))
> + if (atomisp_css_start(asd, css_pipe_id, true)) {
> dev_warn(isp->dev,
> "start SP failed, so do not set streaming to be enable!\n");
> - else
> + } else {
> + spin_lock_irqsave(&isp->lock, flags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
> + spin_unlock_irqrestore(&isp->lock, flags);
> + }
>
> atomisp_csi2_configure(asd);
> }
> @@ -1608,6 +1620,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
> {
> int i;
>
> + lockdep_assert_held(&isp->mutex);
> +
> if (!atomisp_streaming_count(isp))
> return;
>
> @@ -4046,6 +4060,8 @@ void atomisp_handle_parameter_and_buffer(struct atomisp_video_pipe *pipe)
> unsigned long irqflags;
> bool need_to_enqueue_buffer = false;
>
> + lockdep_assert_held(&asd->isp->mutex);
> +
> if (!asd) {
> dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
> __func__, pipe->vdev.name);
> @@ -4139,6 +4155,8 @@ int atomisp_set_parameters(struct video_device *vdev,
> struct atomisp_css_params *css_param = &asd->params.css_param;
> int ret;
>
> + lockdep_assert_held(&asd->isp->mutex);
> +
> if (!asd) {
> dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
> __func__, vdev->name);
> @@ -5537,6 +5555,8 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
> struct v4l2_subdev_fh fh;
> int ret;
>
> + lockdep_assert_held(&isp->mutex);
> +
> if (!asd) {
> dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
> __func__, vdev->name);
> @@ -6159,6 +6179,8 @@ int atomisp_offline_capture_configure(struct atomisp_sub_device *asd,
> {
> struct v4l2_ctrl *c;
>
> + lockdep_assert_held(&asd->isp->mutex);
> +
> /*
> * In case of M10MO ZSL capture case, we need to issue a separate
> * capture request to M10MO which will output captured jpeg image
> @@ -6433,6 +6455,8 @@ int atomisp_exp_id_capture(struct atomisp_sub_device *asd, int *exp_id)
> int value = *exp_id;
> int ret;
>
> + lockdep_assert_held(&isp->mutex);
> +
> ret = __is_raw_buffer_locked(asd, value);
> if (ret) {
> dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
> @@ -6454,6 +6478,8 @@ int atomisp_exp_id_unlock(struct atomisp_sub_device *asd, int *exp_id)
> int value = *exp_id;
> int ret;
>
> + lockdep_assert_held(&isp->mutex);
> +
> ret = __clear_raw_buffer_bitmap(asd, value);
> if (ret) {
> dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
> @@ -6489,6 +6515,8 @@ int atomisp_inject_a_fake_event(struct atomisp_sub_device *asd, int *event)
> if (!event || asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
> return -EINVAL;
>
> + lockdep_assert_held(&asd->isp->mutex);
> +
> dev_dbg(asd->isp->dev, "%s: trying to inject a fake event 0x%x\n",
> __func__, *event);
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> index cda0b5eba16d..15ef31b0c601 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> @@ -3626,6 +3626,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
> struct atomisp_dis_buf *dis_buf;
> unsigned long flags;
>
> + lockdep_assert_held(&isp->mutex);
> +
> if (!asd->params.dvs_stat->hor_prod.odd_real ||
> !asd->params.dvs_stat->hor_prod.odd_imag ||
> !asd->params.dvs_stat->hor_prod.even_real ||
> @@ -3637,12 +3639,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
> return -EINVAL;
>
> /* isp needs to be streaming to get DIS statistics */
> - spin_lock_irqsave(&isp->lock, flags);
> - if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED) {
> - spin_unlock_irqrestore(&isp->lock, flags);
> + if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
> return -EINVAL;
> - }
> - spin_unlock_irqrestore(&isp->lock, flags);
>
> if (atomisp_compare_dvs_grid(asd, &stats->dvs2_stat.grid_info) != 0)
> /* If the grid info in the argument differs from the current
> @@ -3801,6 +3799,8 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
> bool reset_wdt_timer[MAX_STREAM_NUM] = {false};
> int i;
>
> + lockdep_assert_held(&isp->mutex);
> +
> while (!ia_css_dequeue_psys_event(¤t_event.event)) {
> if (current_event.event.type ==
> IA_CSS_EVENT_TYPE_FW_ASSERT) {
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> index 57587d739c4b..e1b213ba4686 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> @@ -813,6 +813,7 @@ static int atomisp_release(struct file *file)
> struct v4l2_requestbuffers req;
> struct v4l2_subdev_fh fh;
> struct v4l2_rect clear_compose = {0};
> + unsigned long flags;
> int ret = 0;
>
> v4l2_fh_init(&fh.vfh, vdev);
> @@ -878,7 +879,9 @@ static int atomisp_release(struct file *file)
>
> /* clear the asd field to show this camera is not used */
> isp->inputs[asd->input_curr].asd = NULL;
> + spin_lock_irqsave(&isp->lock, flags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
> + spin_unlock_irqrestore(&isp->lock, flags);
>
> if (atomisp_dev_users(isp))
> goto done;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> index 759575cbd356..b2c362ef7199 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> @@ -266,7 +266,7 @@ struct atomisp_device {
>
> atomic_t wdt_work_queued;
>
> - spinlock_t lock; /* Just for streaming below */
> + spinlock_t lock; /* Protects asd[i].streaming */
>
> bool need_gfx_throttle;
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index 4016ac4fffe0..21af5feca386 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -1837,7 +1837,9 @@ static int atomisp_streamon(struct file *file, void *fh,
> if (ret)
> goto out;
>
> + spin_lock_irqsave(&isp->lock, irqflags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
> + spin_unlock_irqrestore(&isp->lock, irqflags);
> atomic_set(&asd->sof_count, -1);
> atomic_set(&asd->sequence, -1);
> atomic_set(&asd->sequence_temp, -1);
> @@ -1910,7 +1912,9 @@ static int atomisp_streamon(struct file *file, void *fh,
> ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> video, s_stream, 1);
> if (ret) {
> + spin_lock_irqsave(&isp->lock, irqflags);
> asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
> + spin_unlock_irqrestore(&isp->lock, irqflags);
> ret = -EINVAL;
> goto out;
> }
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index 4a4367701509..88bf693f4c50 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -874,12 +874,18 @@ static int s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct atomisp_sub_device *asd = container_of(
> ctrl->handler, struct atomisp_sub_device, ctrl_handler);
> + unsigned int streaming;
> + unsigned long flags;
>
> switch (ctrl->id) {
> case V4L2_CID_RUN_MODE:
> return __atomisp_update_run_mode(asd);
> case V4L2_CID_DEPTH_MODE:
> - if (asd->streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
> + /* Use spinlock instead of mutex to avoid possible locking issues */
> + spin_lock_irqsave(&asd->isp->lock, flags);
> + streaming = asd->streaming;
> + spin_unlock_irqrestore(&asd->isp->lock, flags);
> + if (streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
> dev_err(asd->isp->dev,
> "ISP is streaming, it is not supported to change the depth mode\n");
> return -EINVAL;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> index eaf767880407..b44f060b0bb5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> @@ -330,7 +330,11 @@ struct atomisp_sub_device {
> atomic_t sequence; /* Sequence value that is assigned to buffer. */
> atomic_t sequence_temp;
>
> - unsigned int streaming; /* Hold both mutex and lock to change this */
> + /*
> + * Writers of streaming must hold both isp->mutex and isp->lock.
> + * Readers of streaming need to hold only one of the two locks.
> + */
> + unsigned int streaming;
> bool stream_prepared; /* whether css stream is created */
>
> /* subdev index: will be used to show which subdev is holding the
> --
> 2.37.3
>
--
With Best Regards,
Andy Shevchenko
