Re: [PATCH v2] media: stm32-dcmi: fix lock scheme

[Hugues FRUCHET]

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 
>> ---
>> 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(>irqlock);
>> +spin_lock_irq(>irqlock);
>>   
>>  /* Check DMA status */
>>  status = dmaengine_tx_status(chan, dcmi->dma_cookie, );
>> @@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
>>  dcmi->errors_count++;
>>  dcmi->active = NULL;
>>   
>> -spin_unlock(>irqlock);
>> +spin_unlock_irq(>irqlock);
>>  return;
>>  }
>>   
>> @@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
>>   
>>  list_del_init(>active->list);
>>   
>> -if (dcmi_start_capture(dcmi)) {
>> +spin_unlock_irq(>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(>irqlock);
>> -return;
>> -}
>> +return;
>>  }
>>   
>>  break;
>> @@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
>>  break;
>>  }
>>   
>> -spin_unlock(>irqlock);
>> +spin_unlock_irq(>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(>irqlock);
>> +spin_lock_irq(>irqlock);
>>   
>>  /* Stop capture is required */
>>  if (dcmi->state == STOPPING) {
>> @@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>>   
>>  complete(>complete);
>>   
>> -spin_unlock(>irqlock);
>> +spin_unlock_irq(>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(>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(>irqlock);
>> -return IRQ_HANDLED;
>> -}
>> +return IRQ_HANDLED;
>>  }
>>   
>> -spin_unlock(>irqlock);
>> +spin_unlock_irq(>irqlock);
>>  return IRQ_HANDLED;
>>   }
>>   
>>   static irqreturn_t dcmi_irq_callback(int irq, void *arg)
>>   {
>>  struct stm32_dcmi *dcmi = arg;
>> +unsigned long flags;
>>   
>> -spin_lock(>irqlock);
>> +spin_lock_irqsave(>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(>irqlock);
>> +spin_unlock_irqrestore(>irqlock, flags);
>>   
>>  return IRQ_WAKE_THREAD;
>>   }
>> @@ -490,9 +487,8 @@ static void dcmi_buf_queue(struct 

Re: [PATCH v2] media: stm32-dcmi: fix lock scheme

[Hans Verkuil]

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 
> ---
> 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(>irqlock);
> + spin_lock_irq(>irqlock);
>  
>   /* Check DMA status */
>   status = dmaengine_tx_status(chan, dcmi->dma_cookie, );
> @@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
>   dcmi->errors_count++;
>   dcmi->active = NULL;
>  
> - spin_unlock(>irqlock);
> + spin_unlock_irq(>irqlock);
>   return;
>   }
>  
> @@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
>  
>   list_del_init(>active->list);
>  
> - if (dcmi_start_capture(dcmi)) {
> + spin_unlock_irq(>irqlock);
> + if (dcmi_start_capture(dcmi))
>   dev_err(dcmi->dev, "%s: Cannot restart capture 
> on DMA complete\n",
>   __func__);
> -
> - spin_unlock(>irqlock);
> - return;
> - }
> + return;
>   }
>  
>   break;
> @@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
>   break;
>   }
>  
> - spin_unlock(>irqlock);
> + spin_unlock_irq(>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(>irqlock);
> + spin_lock_irq(>irqlock);
>  
>   /* Stop capture is required */
>   if (dcmi->state == STOPPING) {
> @@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
>  
>   complete(>complete);
>  
> - spin_unlock(>irqlock);
> + spin_unlock_irq(>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(>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(>irqlock);
> - return IRQ_HANDLED;
> - }
> + return IRQ_HANDLED;
>   }
>  
> - spin_unlock(>irqlock);
> + spin_unlock_irq(>irqlock);
>   return IRQ_HANDLED;
>  }
>  
>  static irqreturn_t dcmi_irq_callback(int irq, void *arg)
>  {
>   struct stm32_dcmi *dcmi = arg;
> + unsigned long flags;
>  
> - spin_lock(>irqlock);
> + spin_lock_irqsave(>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(>irqlock);
> + spin_unlock_irqrestore(>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(>irqlock, flags);
> + spin_lock_irq(>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);
>  
> - 

[PATCH v2] media: stm32-dcmi: fix lock scheme

[Hugues Fruchet]

Fix lock scheme leading to spurious freeze.

Signed-off-by: Hugues Fruchet 
---
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(>irqlock);
+   spin_lock_irq(>irqlock);
 
/* Check DMA status */
status = dmaengine_tx_status(chan, dcmi->dma_cookie, );
@@ -239,7 +239,7 @@ static void dcmi_dma_callback(void *param)
dcmi->errors_count++;
dcmi->active = NULL;
 
-   spin_unlock(>irqlock);
+   spin_unlock_irq(>irqlock);
return;
}
 
@@ -248,13 +248,11 @@ static void dcmi_dma_callback(void *param)
 
list_del_init(>active->list);
 
-   if (dcmi_start_capture(dcmi)) {
+   spin_unlock_irq(>irqlock);
+   if (dcmi_start_capture(dcmi))
dev_err(dcmi->dev, "%s: Cannot restart capture 
on DMA complete\n",
__func__);
-
-   spin_unlock(>irqlock);
-   return;
-   }
+   return;
}
 
break;
@@ -263,7 +261,7 @@ static void dcmi_dma_callback(void *param)
break;
}
 
-   spin_unlock(>irqlock);
+   spin_unlock_irq(>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(>irqlock);
+   spin_lock_irq(>irqlock);
 
/* Stop capture is required */
if (dcmi->state == STOPPING) {
@@ -370,7 +368,7 @@ static irqreturn_t dcmi_irq_thread(int irq, void *arg)
 
complete(>complete);
 
-   spin_unlock(>irqlock);
+   spin_unlock_irq(>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(>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(>irqlock);
-   return IRQ_HANDLED;
-   }
+   return IRQ_HANDLED;
}
 
-   spin_unlock(>irqlock);
+   spin_unlock_irq(>irqlock);
return IRQ_HANDLED;
 }
 
 static irqreturn_t dcmi_irq_callback(int irq, void *arg)
 {
struct stm32_dcmi *dcmi = arg;
+   unsigned long flags;
 
-   spin_lock(>irqlock);
+   spin_lock_irqsave(>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(>irqlock);
+   spin_unlock_irqrestore(>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(>irqlock, flags);
+   spin_lock_irq(>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(>irqlock);
+   if (dcmi_start_capture(dcmi))
dev_err(dcmi->dev, "%s: Cannot restart capture on 
overflow or error\n",
__func__);
-
-   spin_unlock_irqrestore(>irqlock,