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

Tomi Valkeinen
2023-12-06T10:08:33Z

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