linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] media: stm32-dcmi: fix lock scheme
@ 2018-02-22  9:49 Hugues Fruchet
  2018-02-26 13:56 ` Hans Verkuil
  0 siblings, 1 reply; 3+ messages in thread
From: Hugues Fruchet @ 2018-02-22  9:49 UTC (permalink / raw)
  To: Maxime Coquelin, Alexandre Torgue, Mauro Carvalho Chehab, Hans Verkuil
  Cc: linux-media, linux-arm-kernel, linux-kernel, Benjamin Gaignard,
	Yannick Fertre, Hugues Fruchet

Fix lock scheme leading to spurious freeze.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
version 2:
  - dcmi_buf_queue() refactor to avoid to have "else" after "return"
    (warning detected by checkpatch.pl --strict -f stm32-dcmi.c)

 drivers/media/platform/stm32/stm32-dcmi.c | 57 +++++++++++++------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 2fd8bed..5de18ad 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -197,7 +197,7 @@ static void dcmi_dma_callback(void *param)
 	struct dma_tx_state state;
 	enum dma_status status;
 
-	spin_lock(&dcmi->irqlock);
+	spin_lock_irq(&dcmi->irqlock);
 
 	/* Check DMA status */
 	status = dmaengine_tx_status(chan, dcmi->dma_cookie, &state);
@@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
 				dcmi->errors_count++;
 				dcmi->active = NULL;
 
-				spin_unlock(&dcmi->irqlock);
+				spin_unlock_irq(&dcmi->irqlock);
 				return;
 			}
 
@@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
 
 			list_del_init(&dcmi->active->list);
 
-			if (dcmi_start_capture(dcmi)) {
+			spin_unlock_irq(&dcmi->irqlock);
+			if (dcmi_start_capture(dcmi))
 				dev_err(dcmi->dev, "%s: Cannot restart capture on DMA complete\n",
 					__func__);
-
-				spin_unlock(&dcmi->irqlock);
-				return;
-			}
+			return;
 		}
 
 		break;
@@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
 		break;
 	}
 
-	spin_unlock(&dcmi->irqlock);
+	spin_unlock_irq(&dcmi->irqlock);
 }
 
 static int dcmi_start_dma(struct stm32_dcmi *dcmi,
@@ -360,7 +358,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
 {
 	struct stm32_dcmi *dcmi = arg;
 
-	spin_lock(&dcmi->irqlock);
+	spin_lock_irq(&dcmi->irqlock);
 
 	/* Stop capture is required */
 	if (dcmi->state == STOPPING) {
@@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
 
 		complete(&dcmi->complete);
 
-		spin_unlock(&dcmi->irqlock);
+		spin_unlock_irq(&dcmi->irqlock);
 		return IRQ_HANDLED;
 	}
 
@@ -383,35 +381,34 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
 			 __func__);
 
 		dcmi->errors_count++;
-		dmaengine_terminate_all(dcmi->dma_chan);
-
 		dev_dbg(dcmi->dev, "Restarting capture after DCMI error\n");
 
-		if (dcmi_start_capture(dcmi)) {
+		spin_unlock_irq(&dcmi->irqlock);
+		dmaengine_terminate_all(dcmi->dma_chan);
+
+		if (dcmi_start_capture(dcmi))
 			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
 				__func__);
-
-			spin_unlock(&dcmi->irqlock);
-			return IRQ_HANDLED;
-		}
+		return IRQ_HANDLED;
 	}
 
-	spin_unlock(&dcmi->irqlock);
+	spin_unlock_irq(&dcmi->irqlock);
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t dcmi_irq_callback(int irq, void *arg)
 {
 	struct stm32_dcmi *dcmi = arg;
+	unsigned long flags;
 
-	spin_lock(&dcmi->irqlock);
+	spin_lock_irqsave(&dcmi->irqlock, flags);
 
 	dcmi->misr = reg_read(dcmi->regs, DCMI_MIS);
 
 	/* Clear interrupt */
 	reg_set(dcmi->regs, DCMI_ICR, IT_FRAME | IT_OVR | IT_ERR);
 
-	spin_unlock(&dcmi->irqlock);
+	spin_unlock_irqrestore(&dcmi->irqlock, flags);
 
 	return IRQ_WAKE_THREAD;
 }
@@ -490,9 +487,8 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
 	struct stm32_dcmi *dcmi =  vb2_get_drv_priv(vb->vb2_queue);
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct dcmi_buf *buf = container_of(vbuf, struct dcmi_buf, vb);
-	unsigned long flags = 0;
 
-	spin_lock_irqsave(&dcmi->irqlock, flags);
+	spin_lock_irq(&dcmi->irqlock);
 
 	if ((dcmi->state == RUNNING) && (!dcmi->active)) {
 		dcmi->active = buf;
@@ -500,19 +496,15 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
 		dev_dbg(dcmi->dev, "Starting capture on buffer[%d] queued\n",
 			buf->vb.vb2_buf.index);
 
-		if (dcmi_start_capture(dcmi)) {
+		spin_unlock_irq(&dcmi->irqlock);
+		if (dcmi_start_capture(dcmi))
 			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
 				__func__);
-
-			spin_unlock_irqrestore(&dcmi->irqlock, flags);
-			return;
-		}
 	} else {
 		/* Enqueue to video buffers list */
 		list_add_tail(&buf->list, &dcmi->buffers);
+		spin_unlock_irq(&dcmi->irqlock);
 	}
-
-	spin_unlock_irqrestore(&dcmi->irqlock, flags);
 }
 
 static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
@@ -598,20 +590,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	dev_dbg(dcmi->dev, "Start streaming, starting capture\n");
 
+	spin_unlock_irq(&dcmi->irqlock);
 	ret = dcmi_start_capture(dcmi);
 	if (ret) {
 		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
 			__func__);
-
-		spin_unlock_irq(&dcmi->irqlock);
 		goto err_subdev_streamoff;
 	}
 
 	/* Enable interruptions */
 	reg_set(dcmi->regs, DCMI_IER, IT_FRAME | IT_OVR | IT_ERR);
 
-	spin_unlock_irq(&dcmi->irqlock);
-
 	return 0;
 
 err_subdev_streamoff:
@@ -654,7 +643,9 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
 		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
 			__func__, ret);
 
+	spin_lock_irq(&dcmi->irqlock);
 	dcmi->state = STOPPING;
+	spin_unlock_irq(&dcmi->irqlock);
 
 	timeout = wait_for_completion_interruptible_timeout(&dcmi->complete,
 							    time_ms);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: stm32-dcmi: fix lock scheme
  2018-02-22  9:49 [PATCH v2] media: stm32-dcmi: fix lock scheme Hugues Fruchet
@ 2018-02-26 13:56 ` Hans Verkuil
  2018-02-28 17:04   ` Hugues FRUCHET
  0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2018-02-26 13:56 UTC (permalink / raw)
  To: Hugues Fruchet, Maxime Coquelin, Alexandre Torgue, Mauro Carvalho Chehab
  Cc: linux-media, linux-arm-kernel, linux-kernel, Benjamin Gaignard,
	Yannick Fertre

On 02/22/2018 10:49 AM, Hugues Fruchet wrote:
> Fix lock scheme leading to spurious freeze.

Can you elaborate a bit more? It's hard to review since you don't
describe what was wrong and why this fixes the problem.

Regards,

	Hans

> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> version 2:
>   - dcmi_buf_queue() refactor to avoid to have "else" after "return"
>     (warning detected by checkpatch.pl --strict -f stm32-dcmi.c)
> 
>  drivers/media/platform/stm32/stm32-dcmi.c | 57 +++++++++++++------------------
>  1 file changed, 24 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 2fd8bed..5de18ad 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -197,7 +197,7 @@ static void dcmi_dma_callback(void *param)
>  	struct dma_tx_state state;
>  	enum dma_status status;
>  
> -	spin_lock(&dcmi->irqlock);
> +	spin_lock_irq(&dcmi->irqlock);
>  
>  	/* Check DMA status */
>  	status = dmaengine_tx_status(chan, dcmi->dma_cookie, &state);
> @@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
>  				dcmi->errors_count++;
>  				dcmi->active = NULL;
>  
> -				spin_unlock(&dcmi->irqlock);
> +				spin_unlock_irq(&dcmi->irqlock);
>  				return;
>  			}
>  
> @@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
>  
>  			list_del_init(&dcmi->active->list);
>  
> -			if (dcmi_start_capture(dcmi)) {
> +			spin_unlock_irq(&dcmi->irqlock);
> +			if (dcmi_start_capture(dcmi))
>  				dev_err(dcmi->dev, "%s: Cannot restart capture on DMA complete\n",
>  					__func__);
> -
> -				spin_unlock(&dcmi->irqlock);
> -				return;
> -			}
> +			return;
>  		}
>  
>  		break;
> @@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
>  		break;
>  	}
>  
> -	spin_unlock(&dcmi->irqlock);
> +	spin_unlock_irq(&dcmi->irqlock);
>  }
>  
>  static int dcmi_start_dma(struct stm32_dcmi *dcmi,
> @@ -360,7 +358,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>  {
>  	struct stm32_dcmi *dcmi = arg;
>  
> -	spin_lock(&dcmi->irqlock);
> +	spin_lock_irq(&dcmi->irqlock);
>  
>  	/* Stop capture is required */
>  	if (dcmi->state == STOPPING) {
> @@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>  
>  		complete(&dcmi->complete);
>  
> -		spin_unlock(&dcmi->irqlock);
> +		spin_unlock_irq(&dcmi->irqlock);
>  		return IRQ_HANDLED;
>  	}
>  
> @@ -383,35 +381,34 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>  			 __func__);
>  
>  		dcmi->errors_count++;
> -		dmaengine_terminate_all(dcmi->dma_chan);
> -
>  		dev_dbg(dcmi->dev, "Restarting capture after DCMI error\n");
>  
> -		if (dcmi_start_capture(dcmi)) {
> +		spin_unlock_irq(&dcmi->irqlock);
> +		dmaengine_terminate_all(dcmi->dma_chan);
> +
> +		if (dcmi_start_capture(dcmi))
>  			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
>  				__func__);
> -
> -			spin_unlock(&dcmi->irqlock);
> -			return IRQ_HANDLED;
> -		}
> +		return IRQ_HANDLED;
>  	}
>  
> -	spin_unlock(&dcmi->irqlock);
> +	spin_unlock_irq(&dcmi->irqlock);
>  	return IRQ_HANDLED;
>  }
>  
>  static irqreturn_t dcmi_irq_callback(int irq, void *arg)
>  {
>  	struct stm32_dcmi *dcmi = arg;
> +	unsigned long flags;
>  
> -	spin_lock(&dcmi->irqlock);
> +	spin_lock_irqsave(&dcmi->irqlock, flags);
>  
>  	dcmi->misr = reg_read(dcmi->regs, DCMI_MIS);
>  
>  	/* Clear interrupt */
>  	reg_set(dcmi->regs, DCMI_ICR, IT_FRAME | IT_OVR | IT_ERR);
>  
> -	spin_unlock(&dcmi->irqlock);
> +	spin_unlock_irqrestore(&dcmi->irqlock, flags);
>  
>  	return IRQ_WAKE_THREAD;
>  }
> @@ -490,9 +487,8 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>  	struct stm32_dcmi *dcmi =  vb2_get_drv_priv(vb->vb2_queue);
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct dcmi_buf *buf = container_of(vbuf, struct dcmi_buf, vb);
> -	unsigned long flags = 0;
>  
> -	spin_lock_irqsave(&dcmi->irqlock, flags);
> +	spin_lock_irq(&dcmi->irqlock);
>  
>  	if ((dcmi->state == RUNNING) && (!dcmi->active)) {
>  		dcmi->active = buf;
> @@ -500,19 +496,15 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>  		dev_dbg(dcmi->dev, "Starting capture on buffer[%d] queued\n",
>  			buf->vb.vb2_buf.index);
>  
> -		if (dcmi_start_capture(dcmi)) {
> +		spin_unlock_irq(&dcmi->irqlock);
> +		if (dcmi_start_capture(dcmi))
>  			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
>  				__func__);
> -
> -			spin_unlock_irqrestore(&dcmi->irqlock, flags);
> -			return;
> -		}
>  	} else {
>  		/* Enqueue to video buffers list */
>  		list_add_tail(&buf->list, &dcmi->buffers);
> +		spin_unlock_irq(&dcmi->irqlock);
>  	}
> -
> -	spin_unlock_irqrestore(&dcmi->irqlock, flags);
>  }
>  
>  static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> @@ -598,20 +590,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	dev_dbg(dcmi->dev, "Start streaming, starting capture\n");
>  
> +	spin_unlock_irq(&dcmi->irqlock);
>  	ret = dcmi_start_capture(dcmi);
>  	if (ret) {
>  		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
>  			__func__);
> -
> -		spin_unlock_irq(&dcmi->irqlock);
>  		goto err_subdev_streamoff;
>  	}
>  
>  	/* Enable interruptions */
>  	reg_set(dcmi->regs, DCMI_IER, IT_FRAME | IT_OVR | IT_ERR);
>  
> -	spin_unlock_irq(&dcmi->irqlock);
> -
>  	return 0;
>  
>  err_subdev_streamoff:
> @@ -654,7 +643,9 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>  		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
>  			__func__, ret);
>  
> +	spin_lock_irq(&dcmi->irqlock);
>  	dcmi->state = STOPPING;
> +	spin_unlock_irq(&dcmi->irqlock);
>  
>  	timeout = wait_for_completion_interruptible_timeout(&dcmi->complete,
>  							    time_ms);
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: stm32-dcmi: fix lock scheme
  2018-02-26 13:56 ` Hans Verkuil
@ 2018-02-28 17:04   ` Hugues FRUCHET
  0 siblings, 0 replies; 3+ messages in thread
From: Hugues FRUCHET @ 2018-02-28 17:04 UTC (permalink / raw)
  To: Hans Verkuil, Maxime Coquelin, Alexandre TORGUE, Mauro Carvalho Chehab
  Cc: linux-media, linux-arm-kernel, linux-kernel, Benjamin Gaignard,
	Yannick FERTRE

Hi Hans,

Problem was that lock was held while calling dmaengine APIs, which 
causes double lock issue when dma callback was called under context of 
dma_async_issue_pending() (see dcmi_start_dma() & below comments).
Rest of changes are around spin_lock() changed to spin_lock_irq() for 
safer lock management.

On 02/26/2018 02:56 PM, Hans Verkuil wrote:
> On 02/22/2018 10:49 AM, Hugues Fruchet wrote:
>> Fix lock scheme leading to spurious freeze.
> 
> Can you elaborate a bit more? It's hard to review since you don't
> describe what was wrong and why this fixes the problem.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>> version 2:
>>    - dcmi_buf_queue() refactor to avoid to have "else" after "return"
>>      (warning detected by checkpatch.pl --strict -f stm32-dcmi.c)
>>
>>   drivers/media/platform/stm32/stm32-dcmi.c | 57 +++++++++++++------------------
>>   1 file changed, 24 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index 2fd8bed..5de18ad 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -197,7 +197,7 @@ static void dcmi_dma_callback(void *param)
>>   	struct dma_tx_state state;
>>   	enum dma_status status;
>>   
>> -	spin_lock(&dcmi->irqlock);
>> +	spin_lock_irq(&dcmi->irqlock);
>>   
>>   	/* Check DMA status */
>>   	status = dmaengine_tx_status(chan, dcmi->dma_cookie, &state);
>> @@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
>>   				dcmi->errors_count++;
>>   				dcmi->active = NULL;
>>   
>> -				spin_unlock(&dcmi->irqlock);
>> +				spin_unlock_irq(&dcmi->irqlock);
>>   				return;
>>   			}
>>   
>> @@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
>>   
>>   			list_del_init(&dcmi->active->list);
>>   
>> -			if (dcmi_start_capture(dcmi)) {
>> +			spin_unlock_irq(&dcmi->irqlock);

Lock is released here before calling dcmi_start_capture() which calls 
dma_async_issue_pending()

>> +			if (dcmi_start_capture(dcmi))
>>   				dev_err(dcmi->dev, "%s: Cannot restart capture on DMA complete\n",
>>   					__func__);
>> -
>> -				spin_unlock(&dcmi->irqlock);
>> -				return;
>> -			}
>> +			return;
>>   		}
>>   
>>   		break;
>> @@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
>>   		break;
>>   	}
>>   
>> -	spin_unlock(&dcmi->irqlock);
>> +	spin_unlock_irq(&dcmi->irqlock);
>>   }
>>   
>>   static int dcmi_start_dma(struct stm32_dcmi *dcmi,
>> @@ -360,7 +358,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>>   {
>>   	struct stm32_dcmi *dcmi = arg;
>>   
>> -	spin_lock(&dcmi->irqlock);
>> +	spin_lock_irq(&dcmi->irqlock);
>>   
>>   	/* Stop capture is required */
>>   	if (dcmi->state == STOPPING) {
>> @@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>>   
>>   		complete(&dcmi->complete);
>>   
>> -		spin_unlock(&dcmi->irqlock);
>> +		spin_unlock_irq(&dcmi->irqlock);
>>   		return IRQ_HANDLED;
>>   	}
>>   
>> @@ -383,35 +381,34 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>>   			 __func__);
>>   
>>   		dcmi->errors_count++;
>> -		dmaengine_terminate_all(dcmi->dma_chan);
>> -
>>   		dev_dbg(dcmi->dev, "Restarting capture after DCMI error\n");
>>   
>> -		if (dcmi_start_capture(dcmi)) {
>> +		spin_unlock_irq(&dcmi->irqlock);
Lock is released here before calling dma APIs (terminate then 
async_issue_pending)

>> +		dmaengine_terminate_all(dcmi->dma_chan);
>> +
>> +		if (dcmi_start_capture(dcmi))
>>   			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
>>   				__func__);
>> -
>> -			spin_unlock(&dcmi->irqlock);
>> -			return IRQ_HANDLED;
>> -		}
>> +		return IRQ_HANDLED;
>>   	}
>>   
>> -	spin_unlock(&dcmi->irqlock);
>> +	spin_unlock_irq(&dcmi->irqlock);
>>   	return IRQ_HANDLED;
>>   }
>>   
>>   static irqreturn_t dcmi_irq_callback(int irq, void *arg)
>>   {
>>   	struct stm32_dcmi *dcmi = arg;
>> +	unsigned long flags;
>>   
>> -	spin_lock(&dcmi->irqlock);
>> +	spin_lock_irqsave(&dcmi->irqlock, flags);
>>   
>>   	dcmi->misr = reg_read(dcmi->regs, DCMI_MIS);
>>   
>>   	/* Clear interrupt */
>>   	reg_set(dcmi->regs, DCMI_ICR, IT_FRAME | IT_OVR | IT_ERR);
>>   
>> -	spin_unlock(&dcmi->irqlock);
>> +	spin_unlock_irqrestore(&dcmi->irqlock, flags);
>>   
>>   	return IRQ_WAKE_THREAD;
>>   }
>> @@ -490,9 +487,8 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>>   	struct stm32_dcmi *dcmi =  vb2_get_drv_priv(vb->vb2_queue);
>>   	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>   	struct dcmi_buf *buf = container_of(vbuf, struct dcmi_buf, vb);
>> -	unsigned long flags = 0;
>>   
>> -	spin_lock_irqsave(&dcmi->irqlock, flags);
>> +	spin_lock_irq(&dcmi->irqlock);
>>   
>>   	if ((dcmi->state == RUNNING) && (!dcmi->active)) {
>>   		dcmi->active = buf;
>> @@ -500,19 +496,15 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>>   		dev_dbg(dcmi->dev, "Starting capture on buffer[%d] queued\n",
>>   			buf->vb.vb2_buf.index);
>>   
>> -		if (dcmi_start_capture(dcmi)) {
>> +		spin_unlock_irq(&dcmi->irqlock);
Lock is released here before calling dma APIs (terminate then 
async_issue_pending)

>> +		if (dcmi_start_capture(dcmi))
>>   			dev_err(dcmi->dev, "%s: Cannot restart capture on overflow or error\n",
>>   				__func__);
>> -
>> -			spin_unlock_irqrestore(&dcmi->irqlock, flags);
>> -			return;
>> -		}
>>   	} else {
>>   		/* Enqueue to video buffers list */
>>   		list_add_tail(&buf->list, &dcmi->buffers);
>> +		spin_unlock_irq(&dcmi->irqlock);
>>   	}
>> -
>> -	spin_unlock_irqrestore(&dcmi->irqlock, flags);
>>   }
>>   
>>   static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>> @@ -598,20 +590,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   
>>   	dev_dbg(dcmi->dev, "Start streaming, starting capture\n");
>>   
>> +	spin_unlock_irq(&dcmi->irqlock);
>>   	ret = dcmi_start_capture(dcmi);
>>   	if (ret) {
>>   		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
>>   			__func__);
>> -
>> -		spin_unlock_irq(&dcmi->irqlock);
>>   		goto err_subdev_streamoff;
>>   	}
>>   
>>   	/* Enable interruptions */
>>   	reg_set(dcmi->regs, DCMI_IER, IT_FRAME | IT_OVR | IT_ERR);
>>   
>> -	spin_unlock_irq(&dcmi->irqlock);
>> -
>>   	return 0;
>>   
>>   err_subdev_streamoff:
>> @@ -654,7 +643,9 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>>   		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
>>   			__func__, ret);
>>   
>> +	spin_lock_irq(&dcmi->irqlock);
>>   	dcmi->state = STOPPING;
>> +	spin_unlock_irq(&dcmi->irqlock);
>>   
>>   	timeout = wait_for_completion_interruptible_timeout(&dcmi->complete,
>>   							    time_ms);
>>
> 

Best regards,
Hugues.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-28 17:05 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22  9:49 [PATCH v2] media: stm32-dcmi: fix lock scheme Hugues Fruchet
2018-02-26 13:56 ` Hans Verkuil
2018-02-28 17:04   ` Hugues FRUCHET

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).