Re: [PATCH 03/17] media: atomisp: Fix locking around asd->streaming read/write

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@linux.intel.com>

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@redhat.com>
> ---
>  .../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(&current_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