Re: [PATCH v2 1/2] media: rcar-isp: distinguish absent cores from registration failures

[Niklas Söderlund]

Hi Myeonghun,

Thanks for your patch.

On 2026-10-14 20:41:58 -0400, Myeonghun Pak wrote:
> risp_core_probe() clears core->base when resource setup fails. Once
> resource setup has succeeded, it leaves core->base set while unwinding
> errors from the remaining registration steps, including destruction of
> core->rpp and release of the VSPX device reference.
> 
> risp_probe() treats every -ENODEV from risp_core_probe() as an absent
> optional core. If a later registration step returns that error, the
> parent probe can continue and subsequently call risp_core_remove() on
> resources that have already been released.
> 
> Only accept -ENODEV as an absent core when core->base is NULL. Otherwise,
> propagate the error through the parent probe's subdevice cleanup. This
> also keeps failures from the later IRQ request in the following patch
> from being mistaken for an absent core.

While it would indeed be nice to propagate the error I don't like this 
solution. It creates multiple places to check, first the return code of 
risp_core_probe() and then implicit variable contents ontop of that.

Would it not be nicer to add an additional return code to 
risp_core_probe()? If it returns 0 all good, if it returns -ENODEV the 
optional device was not described or a new code (maybe -ENXIO or -EIO ?) 
if the device is described but we fail to initialize it.

For this first check return code, then check some other variable 
solution,

Nacked-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>

> 
> This issue was identified during our ongoing research while reviewing
> kernel code.
> 
> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> Changes in v2:
> - Split the -ENODEV handling out of the IRQ lifetime fix, as requested
>   by Niklas Soderlund.
> 
> v1: https://lore.kernel.org/r/20261011214448.2043581-1-mhun512@xxxxxxxxx
> 
> Compile-tested with Clang 22.1.2, W=1 (arm64). No runtime testing.
> 
>  drivers/media/platform/renesas/rcar-isp/csisp.c | 2 ++
>  1 file changed, 2 insertions(+)
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index 53ce47020d170454b3d208bce7f4d6aad76764d3..73bf7e42bad2ac1ec2493cd8ec81003cce05a3dd 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -563,6 +563,8 @@ static int risp_probe(struct platform_device *pdev)
>  		isp->subdev.internal_ops = &risp_cs_internal_ops;
>  		break;
>  	case -ENODEV:
> +		if (isp->core.base)
> +			goto error_subdev;
>  		/* The device don't have an ISP core, that is OK. */
>  		ret = 0;
>  		break;

-- 
Kind Regards,
Niklas Söderlund