Hi,
On 9/12/22 13:26, Andy Shevchenko wrote:
> 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>
Thanks.
> 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?
Correct.
> If there is an IRQ relation, shouldn't we always use spinlock for
> readers?
As the comment says writers always should take both locks. IOW there
are no writers in non-sleeping contexts.
Regards,
Hans
>
>> 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
>>
>
