Re: [PATCH 3/3] media: staging: rkisp1: params: in 'stop_streaming' don't release the lock while returning buffers

[Robin Murphy <robin.murphy@xxxxxxx>]

Hi Dafna,

On 2020-06-25 18:42, Dafna Hirschfeld wrote:
> In the stop_streaming callback 'rkisp1_params_vb2_stop_streaming'
> there is no need to release the lock 'config_lock' and then acquire
> it again at each iteration when returning all buffers.
> This is because the stream is about to end and there is no need
> to let the isr access a buffer.
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
>   drivers/staging/media/rkisp1/rkisp1-params.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index bf006dbeee2d..5169b02731f1 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1488,19 +1488,13 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>   	/* stop params input firstly */
>   	spin_lock_irqsave(&params->config_lock, flags);
>   	params->is_streaming = false;
> -	spin_unlock_irqrestore(&params->config_lock, flags);
>   
>   	for (i = 0; i < RKISP1_ISP_PARAMS_REQ_BUFS_MAX; i++) {
> -		spin_lock_irqsave(&params->config_lock, flags);
>   		if (!list_empty(&params->params)) {
>   			buf = list_first_entry(&params->params,
>   					       struct rkisp1_buffer, queue);
>   			list_del(&buf->queue);
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
>   		} else {
> -			spin_unlock_irqrestore(&params->config_lock,
> -					       flags);
>   			break;
>   		}

Just skimming through out of idle curiosity I was going to comment that 
if you end up with this pattern:

	if (!x) {
		//do stuff
	} else {
		break;
	}

it would be better as:

	if (x)
		break;
	//do stuff

However I then went and looked at the whole function and frankly it's a 
bit of a WTF. As best I could decipher, this whole crazy loop appears to 
be a baroque reinvention of:

	list_for_each_entry_safe(&params->params, ..., buf) {
		list_del(&buf->queue);
		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
	}

(assuming from context that the list should never contain more than 
RKISP1_ISP_PARAMS_REQ_BUFS_MAX entries in the first place)

Robin.

>   
> @@ -1508,6 +1502,7 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>   			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>   		buf = NULL;
>   	}
> +	spin_unlock_irqrestore(&params->config_lock, flags);
>   }
>   
>   static int