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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&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
>>
> 





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux