diff mbox series

[4/4] media: rkisp1: Fix IRQ disable race issue

Message ID 20231205-rkisp-irq-fix-v1-4-f4045c74ba45@ideasonboard.com (mailing list archive)
State New
Headers show
Series media: rkisp1: Fix IRQ related issues | expand

Commit Message

Tomi Valkeinen Dec. 5, 2023, 8:09 a.m. UTC 
In rkisp1_isp_stop() and rkisp1_csi_disable() the driver masks the
interrupts and then apparently assumes that the interrupt handler won't
be running, and proceeds in the stop procedure. This is not the case, as
the interrupt handler can already be running, which would lead to the
ISP being disabled while the interrupt handler handling a captured
frame.

It is not clear to me if this problem causes a real issue, but shutting
down the ISP while an interrupt handler is running sounds rather bad.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++++++++-
 drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Dec. 5, 2023, 12:13 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Tue, Dec 05, 2023 at 10:09:35AM +0200, Tomi Valkeinen wrote:
> In rkisp1_isp_stop() and rkisp1_csi_disable() the driver masks the
> interrupts and then apparently assumes that the interrupt handler won't
> be running, and proceeds in the stop procedure. This is not the case, as
> the interrupt handler can already be running, which would lead to the
> ISP being disabled while the interrupt handler handling a captured
> frame.
> 
> It is not clear to me if this problem causes a real issue, but shutting
> down the ISP while an interrupt handler is running sounds rather bad.

Agreed.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++++++++-
>  drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++++++++---
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> index f6b54654b435..f0cef766fc0c 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> @@ -125,8 +125,20 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
>  	struct rkisp1_device *rkisp1 = csi->rkisp1;
>  	u32 val;
>  
> -	/* Mask and clear interrupts. */
> +	/* Mask MIPI interrupts. */
>  	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMSC, 0);
> +
> +	/* Flush posted writes */
> +	rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
> +
> +	/*
> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
> +	 * even after this, but it will return immediately as the MIPI
> +	 * interrupts have been masked.
> +	 */

This comment will need to be updated if patch 3/4 gets replaced by a
patch that drops IRQF_SHARED.

> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MIPI]);
> +
> +	/* Clear MIPI interrupt status */
>  	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_ICR, ~0);
>  
>  	val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_CTRL);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index d6b8786661ad..a6dd497c884c 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -364,11 +364,25 @@ static void rkisp1_isp_stop(struct rkisp1_isp *isp)
>  	 * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
>  	 * Stop ISP(isp) ->wait for ISP isp off
>  	 */
> -	/* stop and clear MI and ISP interrupts */
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>  
> +	/* Mask MI and ISP interrupts */
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_IMSC, 0);
> +
> +	/* Flush posted writes */
> +	rkisp1_read(rkisp1, RKISP1_CIF_MI_IMSC);
> +
> +	/*
> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
> +	 * even after this, but it will return immediately as the MI and ISP
> +	 * interrupts have been masked.
> +	 */

Same here.

> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_ISP]);
> +	if (rkisp1->irqs[RKISP1_IRQ_ISP] != rkisp1->irqs[RKISP1_IRQ_MI])
> +		synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MI]);

It would be nice if we could avoid the double synchronize_irq() for
platforms where RKISP1_IRQ_MIPI and RKISP1_IRQ_ISP are identical, but I
understand that would be difficult.

> +
> +	/* Clear MI and ISP interrupt status */
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_ICR, ~0);
>  
>  	/* stop ISP */
Tomi Valkeinen Dec. 6, 2023, 10:08 a.m. UTC | #2 
On 05/12/2023 14:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Dec 05, 2023 at 10:09:35AM +0200, Tomi Valkeinen wrote:
>> In rkisp1_isp_stop() and rkisp1_csi_disable() the driver masks the
>> interrupts and then apparently assumes that the interrupt handler won't
>> be running, and proceeds in the stop procedure. This is not the case, as
>> the interrupt handler can already be running, which would lead to the
>> ISP being disabled while the interrupt handler handling a captured
>> frame.
>>
>> It is not clear to me if this problem causes a real issue, but shutting
>> down the ISP while an interrupt handler is running sounds rather bad.
> 
> Agreed.
> 
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++++++++-
>>   drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++++++++---
>>   2 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> index f6b54654b435..f0cef766fc0c 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> @@ -125,8 +125,20 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
>>   	struct rkisp1_device *rkisp1 = csi->rkisp1;
>>   	u32 val;
>>   
>> -	/* Mask and clear interrupts. */
>> +	/* Mask MIPI interrupts. */
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMSC, 0);
>> +
>> +	/* Flush posted writes */
>> +	rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
>> +
>> +	/*
>> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
>> +	 * even after this, but it will return immediately as the MIPI
>> +	 * interrupts have been masked.
>> +	 */
> 
> This comment will need to be updated if patch 3/4 gets replaced by a
> patch that drops IRQF_SHARED.

I don't think it needs an update, as the irq handling is divided into 
multiple parts. The handler here may get called due to a MI or ISP 
interrupt, and vice versa.

  Tomi

> 
>> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MIPI]);
>> +
>> +	/* Clear MIPI interrupt status */
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_ICR, ~0);
>>   
>>   	val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_CTRL);
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> index d6b8786661ad..a6dd497c884c 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> @@ -364,11 +364,25 @@ static void rkisp1_isp_stop(struct rkisp1_isp *isp)
>>   	 * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
>>   	 * Stop ISP(isp) ->wait for ISP isp off
>>   	 */
>> -	/* stop and clear MI and ISP interrupts */
>> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
>> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>>   
>> +	/* Mask MI and ISP interrupts */
>> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MI_IMSC, 0);
>> +
>> +	/* Flush posted writes */
>> +	rkisp1_read(rkisp1, RKISP1_CIF_MI_IMSC);
>> +
>> +	/*
>> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
>> +	 * even after this, but it will return immediately as the MI and ISP
>> +	 * interrupts have been masked.
>> +	 */
> 
> Same here.
> 
>> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_ISP]);
>> +	if (rkisp1->irqs[RKISP1_IRQ_ISP] != rkisp1->irqs[RKISP1_IRQ_MI])
>> +		synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MI]);
> 
> It would be nice if we could avoid the double synchronize_irq() for
> platforms where RKISP1_IRQ_MIPI and RKISP1_IRQ_ISP are identical, but I
> understand that would be difficult.
> 
>> +
>> +	/* Clear MI and ISP interrupt status */
>> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MI_ICR, ~0);
>>   
>>   	/* stop ISP */
>
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
index f6b54654b435..f0cef766fc0c 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
@@ -125,8 +125,20 @@  static void rkisp1_csi_disable(struct rkisp1_csi *csi)
 	struct rkisp1_device *rkisp1 = csi->rkisp1;
 	u32 val;
 
-	/* Mask and clear interrupts. */
+	/* Mask MIPI interrupts. */
 	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMSC, 0);
+
+	/* Flush posted writes */
+	rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
+
+	/*
+	 * Wait until the IRQ handler has ended. The IRQ handler may get called
+	 * even after this, but it will return immediately as the MIPI
+	 * interrupts have been masked.
+	 */
+	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MIPI]);
+
+	/* Clear MIPI interrupt status */
 	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_ICR, ~0);
 
 	val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_CTRL);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index d6b8786661ad..a6dd497c884c 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -364,11 +364,25 @@  static void rkisp1_isp_stop(struct rkisp1_isp *isp)
 	 * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
 	 * Stop ISP(isp) ->wait for ISP isp off
 	 */
-	/* stop and clear MI and ISP interrupts */
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
 
+	/* Mask MI and ISP interrupts */
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
 	rkisp1_write(rkisp1, RKISP1_CIF_MI_IMSC, 0);
+
+	/* Flush posted writes */
+	rkisp1_read(rkisp1, RKISP1_CIF_MI_IMSC);
+
+	/*
+	 * Wait until the IRQ handler has ended. The IRQ handler may get called
+	 * even after this, but it will return immediately as the MI and ISP
+	 * interrupts have been masked.
+	 */
+	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_ISP]);
+	if (rkisp1->irqs[RKISP1_IRQ_ISP] != rkisp1->irqs[RKISP1_IRQ_MI])
+		synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MI]);
+
+	/* Clear MI and ISP interrupt status */
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
 	rkisp1_write(rkisp1, RKISP1_CIF_MI_ICR, ~0);
 
 	/* stop ISP */