Re: [PATCH] media: platform: c3-isp: Register IRQ after video state init

[Keke Li]

Hi Runyu,

Thanks for your patch.

On 8/30/26 22:39, Runyu Xiao wrote:
> [You don't often get email from runyu.xiao@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> [ EXTERNAL EMAIL ]
>
> c3_isp_probe() requests the shared IRQ before
> c3_isp_videos_register() initializes the capture, statistics, and
> parameter state used by the IRQ handler. request_irq() permits the
> handler to run as soon as registration completes, so a frame-end
> interrupt can acquire an uninitialized buffer lock.
>
> Register the video state before requesting the IRQ and free the managed
> IRQ before tearing that state down. Store the IRQ number for the remove
> path and preserve the existing probe error unwinding.
>
> Fixes: fb2e135208f3 ("media: platform: Add C3 ISP driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> ---
>   .../media/platform/amlogic/c3/isp/c3-isp-common.h  |  2 ++
>   drivers/media/platform/amlogic/c3/isp/c3-isp-dev.c | 14 +++++++++-----
>   2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-common.h b/drivers/media/platform/amlogic/c3/isp/c3-isp-common.h
> index cb470802e..56b7d48e1 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-common.h
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-common.h
> @@ -293,6 +293,7 @@ struct c3_isp_info {
>    * @stats: ISP stats device
>    * @params: ISP params device
>    * @caps: array of ISP capture device
> + * @irq: ISP interrupt number
>    * @frm_sequence: used to record frame id
>    * @info: version-specific ISP information
>    */
> @@ -312,6 +313,7 @@ struct c3_isp_device {
>          struct c3_isp_params params;
>          struct c3_isp_capture caps[C3_ISP_NUM_CAP_DEVS];
>
> +       int irq;
>          u32 frm_sequence;
>          const struct c3_isp_info *info;
>   };
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-dev.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-dev.c
> index c3b779f63..66242544e 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-dev.c
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-dev.c
> @@ -330,6 +330,7 @@ static int c3_isp_probe(struct platform_device *pdev)
>          irq = platform_get_irq(pdev, 0);
>          if (irq < 0)
>                  return irq;
> +       isp->irq = irq;
>
>          ret = c3_isp_get_clocks(isp);
>          if (ret)
> @@ -355,18 +356,20 @@ static int c3_isp_probe(struct platform_device *pdev)
>          if (ret)
>                  goto err_resizers_unregister;
>
> -       ret = devm_request_irq(dev, irq,
> -                              c3_isp_irq_handler, IRQF_SHARED,
> -                              dev_driver_string(dev), isp);
> +       ret = c3_isp_videos_register(isp);
>          if (ret)
>                  goto err_nf_unregister;
>
> -       ret = c3_isp_videos_register(isp);
> +       ret = devm_request_irq(dev, irq,
> +                              c3_isp_irq_handler, IRQF_SHARED,
> +                              dev_driver_string(dev), isp);

No nee to reorder c3_isp_videos_register and devm_request_irq.

The ISP is inactive during probe, so there are no spurious interrpts.

>          if (ret)
> -               goto err_nf_unregister;
> +               goto err_videos_unregister;
>
>          return 0;
>
> +err_videos_unregister:
> +       c3_isp_videos_unregister(isp);
>   err_nf_unregister:
>          c3_isp_async_nf_unregister(isp);
>   err_resizers_unregister:
> @@ -384,6 +387,7 @@ static void c3_isp_remove(struct platform_device *pdev)
>   {
>          struct c3_isp_device *isp = platform_get_drvdata(pdev);
>
> +       devm_free_irq(isp->dev, isp->irq, isp);
>          c3_isp_videos_unregister(isp);
>          c3_isp_async_nf_unregister(isp);
>          c3_isp_core_unregister(isp);
> --
> 2.34.1