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

Laurent Pinchart
2023-12-05T12:14:07Z

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 */

--
Regards,

Laurent Pinchart