[PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

[PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

[Linmao Li]

of_parse_phandle() and of_find_device_by_node() both acquire references,
but the ISPCORE probe never releases them. The device node reference is
leaked immediately, and the VSPX device reference is leaked on probe
failures and on driver removal.

Drop the node reference once the platform device has been looked up and
release the device reference with a devm action.

Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/media/platform/renesas/rcar-isp/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
index f3dc52c136120..8dafffdd8de68 100644
--- a/drivers/media/platform/renesas/rcar-isp/core.c
+++ b/drivers/media/platform/renesas/rcar-isp/core.c
@@ -781,6 +781,13 @@ int risp_core_registered(struct rcar_isp_core *core, struct v4l2_subdev *sd)
 	return 0;
 }
 
+static void risp_core_put_device(void *data)
+{
+	struct device *dev = data;
+
+	put_device(dev);
+}
+
 static int risp_core_probe_resources(struct rcar_isp_core *core,
 				     struct platform_device *pdev)
 {
@@ -820,9 +827,15 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
 		return -ENODEV;
 
 	vspx = of_find_device_by_node(of_vspx);
+	of_node_put(of_vspx);
 	if (!vspx)
 		return -ENODEV;
 
+	ret = devm_add_action_or_reset(&pdev->dev, risp_core_put_device,
+				       &vspx->dev);
+	if (ret)
+		return ret;
+
 	/* Attach to VSP-X */
 	core->vspx.dev = &vspx->dev;
 

base-commit: 31152f5b0f8719f92063b8c6196cd5e34106c73d
-- 
2.25.1

[PATCH 2/2] media: rcar-isp: Release ISPCORE resources

[Linmao Li]

v4l2_device_register() takes a reference to the parent device, but the
ISPCORE remove path never calls v4l2_device_unregister(). The reference
is therefore leaked whenever an ISPCORE is removed.

Probe failures after rppx1_create() also return without destroying the
RPPX1 object.

Unregister the V4L2 device and destroy the RPPX1 object on the
corresponding error paths, and unregister the V4L2 device during removal.
v4l2_device_unregister() also unregisters all attached subdevices, so it
replaces the narrower subdevice-only cleanup.

Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/media/platform/renesas/rcar-isp/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
index 8dafffdd8de68..923970fd7841f 100644
--- a/drivers/media/platform/renesas/rcar-isp/core.c
+++ b/drivers/media/platform/renesas/rcar-isp/core.c
@@ -883,17 +883,23 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
 
 	ret = v4l2_device_register(core->dev, &core->v4l2_dev);
 	if (ret)
-		return ret;
+		goto err_destroy_rpp;
 
 	ret = risp_core_create_subdev(core);
 	if (ret)
-		return ret;
+		goto err_unregister_v4l2;
 
 	mutex_init(&core->io_lock);
 	spin_lock_init(&core->lock);
 	INIT_LIST_HEAD(&core->risp_jobs);
 
 	return 0;
+
+err_unregister_v4l2:
+	v4l2_device_unregister(&core->v4l2_dev);
+err_destroy_rpp:
+	rppx1_destroy(core->rpp);
+	return ret;
 }
 
 void risp_core_remove(struct rcar_isp_core *core)
@@ -907,7 +913,7 @@ void risp_core_remove(struct rcar_isp_core *core)
 	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
 		risp_core_io_destroy(&core->io[i]);
 
-	v4l2_device_unregister_subdev(&core->subdev);
+	v4l2_device_unregister(&core->v4l2_dev);
 
 	mutex_destroy(&core->io_lock);
 	rppx1_destroy(core->rpp);
-- 
2.25.1

Re: [PATCH 2/2] media: rcar-isp: Release ISPCORE resources

[Jacopo Mondi]

Hi Linmao Li

On Mon, Aug 03, 2026 at 05:05:53PM +0800, Linmao Li wrote:
> v4l2_device_register() takes a reference to the parent device, but the
> ISPCORE remove path never calls v4l2_device_unregister(). The reference
> is therefore leaked whenever an ISPCORE is removed.
>
> Probe failures after rppx1_create() also return without destroying the
> RPPX1 object.
>
> Unregister the V4L2 device and destroy the RPPX1 object on the
> corresponding error paths, and unregister the V4L2 device during removal.
> v4l2_device_unregister() also unregisters all attached subdevices, so it
> replaces the narrower subdevice-only cleanup.
>
> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")

The driver has not landed in any Linux release, but it has just been
collected for the next merge window.

If your patches get collected as part of the same cycle, I don't think
there's any need for a Fixes tag ?

> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
>  drivers/media/platform/renesas/rcar-isp/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> index 8dafffdd8de68..923970fd7841f 100644
> --- a/drivers/media/platform/renesas/rcar-isp/core.c
> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> @@ -883,17 +883,23 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
>
>  	ret = v4l2_device_register(core->dev, &core->v4l2_dev);
>  	if (ret)
> -		return ret;
> +		goto err_destroy_rpp;
>
>  	ret = risp_core_create_subdev(core);
>  	if (ret)
> -		return ret;
> +		goto err_unregister_v4l2;
>
>  	mutex_init(&core->io_lock);
>  	spin_lock_init(&core->lock);
>  	INIT_LIST_HEAD(&core->risp_jobs);
>
>  	return 0;
> +
> +err_unregister_v4l2:
> +	v4l2_device_unregister(&core->v4l2_dev);
> +err_destroy_rpp:
> +	rppx1_destroy(core->rpp);
> +	return ret;
>  }
>
>  void risp_core_remove(struct rcar_isp_core *core)
> @@ -907,7 +913,7 @@ void risp_core_remove(struct rcar_isp_core *core)
>  	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
>  		risp_core_io_destroy(&core->io[i]);
>
> -	v4l2_device_unregister_subdev(&core->subdev);
> +	v4l2_device_unregister(&core->v4l2_dev);

As v4l2_device_unregister() unregister all subdevs of core->v4l2_dev,
this seems correct to me.

R-Car ISP is a little complicated, in the sense that the core->subdev
gets registered by risp_cs_registered() which is the handler of the
channel selector subdev .registered() callback, and to properly
balance we should unregister it in the (not implemented)
.unregistered() handler.

However, as this is called as part of the driver's remove handler, and
that's what we have at the moment, I guess this is ok

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
>  	mutex_destroy(&core->io_lock);
>  	rppx1_destroy(core->rpp);
> --
> 2.25.1
>
>

Re: [PATCH 2/2] media: rcar-isp: Release ISPCORE resources

[Linmao Li]

Hi Jacopo,

在 2026/8/3 22:17, Jacopo Mondi 写道:
> Hi Linmao Li
>
> On Mon, Aug 03, 2026 at 05:05:53PM +0800, Linmao Li wrote:
>> v4l2_device_register() takes a reference to the parent device, but the
>> ISPCORE remove path never calls v4l2_device_unregister(). The reference
>> is therefore leaked whenever an ISPCORE is removed.
>>
>> Probe failures after rppx1_create() also return without destroying the
>> RPPX1 object.
>>
>> Unregister the V4L2 device and destroy the RPPX1 object on the
>> corresponding error paths, and unregister the V4L2 device during removal.
>> v4l2_device_unregister() also unregisters all attached subdevices, so it
>> replaces the narrower subdevice-only cleanup.
>>
>> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> The driver has not landed in any Linux release, but it has just been
> collected for the next merge window.
>
> If your patches get collected as part of the same cycle, I don't think
> there's any need for a Fixes tag ?

Agreed. I will drop the Fixes tags from both patches when respinning the 
series.


Thanks,
Linmao

>
>> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
>> ---
>>   drivers/media/platform/renesas/rcar-isp/core.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
>> index 8dafffdd8de68..923970fd7841f 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/core.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
>> @@ -883,17 +883,23 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
>>
>>   	ret = v4l2_device_register(core->dev, &core->v4l2_dev);
>>   	if (ret)
>> -		return ret;
>> +		goto err_destroy_rpp;
>>
>>   	ret = risp_core_create_subdev(core);
>>   	if (ret)
>> -		return ret;
>> +		goto err_unregister_v4l2;
>>
>>   	mutex_init(&core->io_lock);
>>   	spin_lock_init(&core->lock);
>>   	INIT_LIST_HEAD(&core->risp_jobs);
>>
>>   	return 0;
>> +
>> +err_unregister_v4l2:
>> +	v4l2_device_unregister(&core->v4l2_dev);
>> +err_destroy_rpp:
>> +	rppx1_destroy(core->rpp);
>> +	return ret;
>>   }
>>
>>   void risp_core_remove(struct rcar_isp_core *core)
>> @@ -907,7 +913,7 @@ void risp_core_remove(struct rcar_isp_core *core)
>>   	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
>>   		risp_core_io_destroy(&core->io[i]);
>>
>> -	v4l2_device_unregister_subdev(&core->subdev);
>> +	v4l2_device_unregister(&core->v4l2_dev);
> As v4l2_device_unregister() unregister all subdevs of core->v4l2_dev,
> this seems correct to me.
>
> R-Car ISP is a little complicated, in the sense that the core->subdev
> gets registered by risp_cs_registered() which is the handler of the
> channel selector subdev .registered() callback, and to properly
> balance we should unregister it in the (not implemented)
> .unregistered() handler.
>
> However, as this is called as part of the driver's remove handler, and
> that's what we have at the moment, I guess this is ok
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
>>   	mutex_destroy(&core->io_lock);
>>   	rppx1_destroy(core->rpp);
>> --
>> 2.25.1
>>
>>

Re: [PATCH 2/2] media: rcar-isp: Release ISPCORE resources

[Niklas Söderlund]

Hello,

On 2026-08-04 10:25:35 +0800, Linmao Li wrote:
> Hi Jacopo,
> 
> 在 2026/8/3 22:17, Jacopo Mondi 写道:
> > Hi Linmao Li
> > 
> > On Mon, Aug 03, 2026 at 05:05:53PM +0800, Linmao Li wrote:
> > > v4l2_device_register() takes a reference to the parent device, but the
> > > ISPCORE remove path never calls v4l2_device_unregister(). The reference
> > > is therefore leaked whenever an ISPCORE is removed.
> > > 
> > > Probe failures after rppx1_create() also return without destroying the
> > > RPPX1 object.
> > > 
> > > Unregister the V4L2 device and destroy the RPPX1 object on the
> > > corresponding error paths, and unregister the V4L2 device during removal.
> > > v4l2_device_unregister() also unregisters all attached subdevices, so it
> > > replaces the narrower subdevice-only cleanup.
> > > 
> > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> > The driver has not landed in any Linux release, but it has just been
> > collected for the next merge window.
> > 
> > If your patches get collected as part of the same cycle, I don't think
> > there's any need for a Fixes tag ?
> 
> Agreed. I will drop the Fixes tags from both patches when respinning the
> series.

With Jacopo's concern addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> 
> Thanks,
> Linmao
> 
> > 
> > > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > > ---
> > >   drivers/media/platform/renesas/rcar-isp/core.c | 12 +++++++++---
> > >   1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> > > index 8dafffdd8de68..923970fd7841f 100644
> > > --- a/drivers/media/platform/renesas/rcar-isp/core.c
> > > +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> > > @@ -883,17 +883,23 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
> > > 
> > >   	ret = v4l2_device_register(core->dev, &core->v4l2_dev);
> > >   	if (ret)
> > > -		return ret;
> > > +		goto err_destroy_rpp;
> > > 
> > >   	ret = risp_core_create_subdev(core);
> > >   	if (ret)
> > > -		return ret;
> > > +		goto err_unregister_v4l2;
> > > 
> > >   	mutex_init(&core->io_lock);
> > >   	spin_lock_init(&core->lock);
> > >   	INIT_LIST_HEAD(&core->risp_jobs);
> > > 
> > >   	return 0;
> > > +
> > > +err_unregister_v4l2:
> > > +	v4l2_device_unregister(&core->v4l2_dev);
> > > +err_destroy_rpp:
> > > +	rppx1_destroy(core->rpp);
> > > +	return ret;
> > >   }
> > > 
> > >   void risp_core_remove(struct rcar_isp_core *core)
> > > @@ -907,7 +913,7 @@ void risp_core_remove(struct rcar_isp_core *core)
> > >   	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
> > >   		risp_core_io_destroy(&core->io[i]);
> > > 
> > > -	v4l2_device_unregister_subdev(&core->subdev);
> > > +	v4l2_device_unregister(&core->v4l2_dev);
> > As v4l2_device_unregister() unregister all subdevs of core->v4l2_dev,
> > this seems correct to me.
> > 
> > R-Car ISP is a little complicated, in the sense that the core->subdev
> > gets registered by risp_cs_registered() which is the handler of the
> > channel selector subdev .registered() callback, and to properly
> > balance we should unregister it in the (not implemented)
> > .unregistered() handler.
> > 
> > However, as this is called as part of the driver's remove handler, and
> > that's what we have at the moment, I guess this is ok
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > >   	mutex_destroy(&core->io_lock);
> > >   	rppx1_destroy(core->rpp);
> > > --
> > > 2.25.1
> > > 
> > > 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

[Jacopo Mondi]

Hello Linmao Li

On Mon, Aug 03, 2026 at 05:05:52PM +0800, Linmao Li wrote:
> of_parse_phandle() and of_find_device_by_node() both acquire references,
> but the ISPCORE probe never releases them. The device node reference is
> leaked immediately, and the VSPX device reference is leaked on probe
> failures and on driver removal.
>
> Drop the node reference once the platform device has been looked up and
> release the device reference with a devm action.
>
> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
>  drivers/media/platform/renesas/rcar-isp/core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> index f3dc52c136120..8dafffdd8de68 100644
> --- a/drivers/media/platform/renesas/rcar-isp/core.c
> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> @@ -781,6 +781,13 @@ int risp_core_registered(struct rcar_isp_core *core, struct v4l2_subdev *sd)
>  	return 0;
>  }
>
> +static void risp_core_put_device(void *data)
> +{
> +	struct device *dev = data;
> +
> +	put_device(dev);
> +}
> +
>  static int risp_core_probe_resources(struct rcar_isp_core *core,
>  				     struct platform_device *pdev)
>  {
> @@ -820,9 +827,15 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
>  		return -ENODEV;
>
>  	vspx = of_find_device_by_node(of_vspx);
> +	of_node_put(of_vspx);

I was about to suggest to declared of_vspx as:

	struct device_node *of_vspx = __free(device_node) = NULL;

But maybe it is not necessary since there's a single call place for
of_node_put().


>  	if (!vspx)
>  		return -ENODEV;
>
> +	ret = devm_add_action_or_reset(&pdev->dev, risp_core_put_device,
> +				       &vspx->dev);
> +	if (ret)
> +		return ret;
> +

For my education: what are the drawbacks of using
devm_add_action_or_reset() instead of releasing core->vspx on probe
failures and _remove() ?

Thanks
  j

>  	/* Attach to VSP-X */
>  	core->vspx.dev = &vspx->dev;
>
>
> base-commit: 31152f5b0f8719f92063b8c6196cd5e34106c73d
> --
> 2.25.1
>
>

Re: [PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

[Linmao Li]

Hi Jacopo,

在 2026/8/3 21:43, Jacopo Mondi 写道:
> Hello Linmao Li
>
> On Mon, Aug 03, 2026 at 05:05:52PM +0800, Linmao Li wrote:
>> of_parse_phandle() and of_find_device_by_node() both acquire references,
>> but the ISPCORE probe never releases them. The device node reference is
>> leaked immediately, and the VSPX device reference is leaked on probe
>> failures and on driver removal.
>>
>> Drop the node reference once the platform device has been looked up and
>> release the device reference with a devm action.
>>
>> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
>> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
>> ---
>>   drivers/media/platform/renesas/rcar-isp/core.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
>> index f3dc52c136120..8dafffdd8de68 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/core.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
>> @@ -781,6 +781,13 @@ int risp_core_registered(struct rcar_isp_core *core, struct v4l2_subdev *sd)
>>   	return 0;
>>   }
>>
>> +static void risp_core_put_device(void *data)
>> +{
>> +	struct device *dev = data;
>> +
>> +	put_device(dev);
>> +}
>> +
>>   static int risp_core_probe_resources(struct rcar_isp_core *core,
>>   				     struct platform_device *pdev)
>>   {
>> @@ -820,9 +827,15 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
>>   		return -ENODEV;
>>
>>   	vspx = of_find_device_by_node(of_vspx);
>> +	of_node_put(of_vspx);
> I was about to suggest to declared of_vspx as:
>
> 	struct device_node *of_vspx = __free(device_node) = NULL;
>
> But maybe it is not necessary since there's a single call place for
> of_node_put().
Agreed. The node is only used to look up the platform device and its
reference is dropped immediately afterwards, so I kept the explicit
of_node_put() to make the lifetime obvious.
>
>
>>   	if (!vspx)
>>   		return -ENODEV;
>>
>> +	ret = devm_add_action_or_reset(&pdev->dev, risp_core_put_device,
>> +				       &vspx->dev);
>> +	if (ret)
>> +		return ret;
>> +
> For my education: what are the drawbacks of using
> devm_add_action_or_reset() instead of releasing core->vspx on probe
> failures and _remove() ?
Explicit cleanup would work as well. I used a devm action to avoid
duplicating the put_device() across the probe error paths and the
remove path.

After the VSPX reference has been acquired, risp_core_probe_resources()
can still fail in vsp1_isp_init(), clk_prepare_enable() or
rppx1_create(). risp_core_probe() clears core->base on those failures,
so risp_core_remove() returns early without performing any cleanup. An
explicit implementation would therefore need a common error path in
addition to the put_device() in remove.

The drawbacks of the devm action are the additional devres allocation
and the less explicit release ordering. There is also a longer
reference lifetime in the optional-ISPCORE case: if rppx1_create()
fails with -ENODEV, the parent driver treats the ISP core as absent and
continues probing successfully, so the action is not unwound and the
VSPX reference is retained until the parent device is removed. This is
harmless, but explicit cleanup would release it earlier.

The action is registered after the reset, clock and IRQ devres, so its
put_device() runs before those are released due to the LIFO ordering.
There is no dependency between them.

I chose devm to keep the cleanup centralized, but I can switch to an
explicit error path if you prefer.

Thanks,
Linmao
>
> Thanks
>    j
>
>>   	/* Attach to VSP-X */
>>   	core->vspx.dev = &vspx->dev;
>>
>>
>> base-commit: 31152f5b0f8719f92063b8c6196cd5e34106c73d
>> --
>> 2.25.1
>>
>>

Re: [PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

[Niklas Söderlund]

Hello Linmao,

Thanks for your work.

On 2026-08-04 10:05:03 +0800, Linmao Li wrote:
> Hi Jacopo,
> 
> 在 2026/8/3 21:43, Jacopo Mondi 写道:
> > Hello Linmao Li
> > 
> > On Mon, Aug 03, 2026 at 05:05:52PM +0800, Linmao Li wrote:
> > > of_parse_phandle() and of_find_device_by_node() both acquire references,
> > > but the ISPCORE probe never releases them. The device node reference is
> > > leaked immediately, and the VSPX device reference is leaked on probe
> > > failures and on driver removal.
> > > 
> > > Drop the node reference once the platform device has been looked up and
> > > release the device reference with a devm action.
> > > 
> > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> > > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > > ---
> > >   drivers/media/platform/renesas/rcar-isp/core.c | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> > > index f3dc52c136120..8dafffdd8de68 100644
> > > --- a/drivers/media/platform/renesas/rcar-isp/core.c
> > > +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> > > @@ -781,6 +781,13 @@ int risp_core_registered(struct rcar_isp_core *core, struct v4l2_subdev *sd)
> > >   	return 0;
> > >   }
> > > 
> > > +static void risp_core_put_device(void *data)
> > > +{
> > > +	struct device *dev = data;
> > > +
> > > +	put_device(dev);
> > > +}
> > > +
> > >   static int risp_core_probe_resources(struct rcar_isp_core *core,
> > >   				     struct platform_device *pdev)
> > >   {
> > > @@ -820,9 +827,15 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
> > >   		return -ENODEV;
> > > 
> > >   	vspx = of_find_device_by_node(of_vspx);
> > > +	of_node_put(of_vspx);
> > I was about to suggest to declared of_vspx as:
> > 
> > 	struct device_node *of_vspx = __free(device_node) = NULL;
> > 
> > But maybe it is not necessary since there's a single call place for
> > of_node_put().
> Agreed. The node is only used to look up the platform device and its
> reference is dropped immediately afterwards, so I kept the explicit
> of_node_put() to make the lifetime obvious.
> > 
> > 
> > >   	if (!vspx)
> > >   		return -ENODEV;
> > > 
> > > +	ret = devm_add_action_or_reset(&pdev->dev, risp_core_put_device,
> > > +				       &vspx->dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > For my education: what are the drawbacks of using
> > devm_add_action_or_reset() instead of releasing core->vspx on probe
> > failures and _remove() ?
> Explicit cleanup would work as well. I used a devm action to avoid
> duplicating the put_device() across the probe error paths and the
> remove path.
> 
> After the VSPX reference has been acquired, risp_core_probe_resources()
> can still fail in vsp1_isp_init(), clk_prepare_enable() or
> rppx1_create(). risp_core_probe() clears core->base on those failures,
> so risp_core_remove() returns early without performing any cleanup. An
> explicit implementation would therefore need a common error path in
> addition to the put_device() in remove.
> 
> The drawbacks of the devm action are the additional devres allocation
> and the less explicit release ordering. There is also a longer
> reference lifetime in the optional-ISPCORE case: if rppx1_create()
> fails with -ENODEV, the parent driver treats the ISP core as absent and
> continues probing successfully, so the action is not unwound and the
> VSPX reference is retained until the parent device is removed. This is
> harmless, but explicit cleanup would release it earlier.
> 
> The action is registered after the reset, clock and IRQ devres, so its
> put_device() runs before those are released due to the LIFO ordering.
> There is no dependency between them.
> 
> I chose devm to keep the cleanup centralized, but I can switch to an
> explicit error path if you prefer.

I thin I would prefers an explicit error path. Specially as you point 
out, the driver can work with or without an ISPCORE and having the error 
path explicit will make things more robust IMHO.

> 
> Thanks,
> Linmao
> > 
> > Thanks
> >    j
> > 
> > >   	/* Attach to VSP-X */
> > >   	core->vspx.dev = &vspx->dev;
> > > 
> > > 
> > > base-commit: 31152f5b0f8719f92063b8c6196cd5e34106c73d
> > > --
> > > 2.25.1
> > > 
> > > 

-- 
Kind Regards,
Niklas Söderlund