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

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 <[email protected]>
>> ---
>>   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 <[email protected]>
>
>>   	mutex_destroy(&core->io_lock);
>>   	rppx1_destroy(core->rpp);
>> --
>> 2.25.1
>>
>>