Re: [PATCH 05/17] media: atomisp: Add atomisp_pipe_check() helper

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

 



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@xxxxxxxxxxxxxxx>
> 
> (One minor question below)
> 
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>>  .../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
>>
> 





[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