Re: [PATCH v3] media: i2c: vd56g3: Clean up subdev state on init failure

[Benjamin Mugnier]

Hi Myeonghun,

Sorry for the delay.

Le 22/06/2026 à 14:14, Myeonghun Pak a écrit :
> vd56g3_subdev_init() calls v4l2_subdev_init_finalize(), which allocates
> the subdev active state and requires v4l2_subdev_cleanup() to release it.
> 
> If vd56g3_update_controls() fails after finalize succeeds, the probe error
> path currently skips v4l2_subdev_cleanup() and returns an error. The driver
> .remove() callback is not called after a failed probe, so the active state
> is leaked.
> 
> Move the control update to vd56g3_probe() after vd56g3_subdev_init()
> succeeds, and route failures through the existing err_subdev path. This
> keeps v4l2_subdev_cleanup() in the common vd56g3_subdev_cleanup() helper.
> 
> Fixes: 87aa97fc3157 ("media: i2c: Add driver for ST VD56G3 camera sensor")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: OpenAI:GPT-5.4
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>

One more nitpick but otherwise :

Reviewed-by: Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx>

> ---
> Changes in v3:
> - Restore the capitalized subject summary.
> - Route vd56g3_update_controls() failures through the probe-side subdev
>   cleanup path instead of adding a second v4l2_subdev_cleanup() call.
> - Add the Assisted-by trailer.
> 
> Changes in v2:
> - Use a lowercase subject summary.
> 
>  drivers/media/i2c/vd56g3.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/vd56g3.c b/drivers/media/i2c/vd56g3.c
> index 157acea9e2..39fb14bab5 100644
> --- a/drivers/media/i2c/vd56g3.c
> +++ b/drivers/media/i2c/vd56g3.c
> @@ -1388,7 +1388,6 @@ static int vd56g3_detect(struct vd56g3 *sensor)
>  
>  static int vd56g3_subdev_init(struct vd56g3 *sensor)
>  {
> -	struct v4l2_subdev_state *state;
>  	int ret;
>  
>  	/* Init remaining sub device ops */
> @@ -1421,15 +1420,6 @@ static int vd56g3_subdev_init(struct vd56g3 *sensor)
>  		goto err_ctrls;
>  	}
>  
> -	/* Update controls according to the resolution set */
> -	state = v4l2_subdev_lock_and_get_active_state(&sensor->sd);
> -	ret = vd56g3_update_controls(sensor);
> -	v4l2_subdev_unlock_state(state);
> -	if (ret) {
> -		dev_err(sensor->dev, "Controls update failed: %d\n", ret);
> -		goto err_ctrls;

The err_ctrls label could be renamed to something more relevant now.

> -	}
> -
>  	return 0;
>  
>  err_ctrls:
> @@ -1452,6 +1442,7 @@ static void vd56g3_subdev_cleanup(struct vd56g3 *sensor)
>  static int vd56g3_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> +	struct v4l2_subdev_state *state;
>  	struct vd56g3 *sensor;
>  	int ret;
>  
> @@ -1517,6 +1508,15 @@ static int vd56g3_probe(struct i2c_client *client)
>  		goto err_power_off;
>  	}
>  
> +	/* Update controls according to the resolution set */
> +	state = v4l2_subdev_lock_and_get_active_state(&sensor->sd);
> +	ret = vd56g3_update_controls(sensor);
> +	v4l2_subdev_unlock_state(state);
> +	if (ret) {
> +		dev_err(sensor->dev, "Controls update failed: %d\n", ret);
> +		goto err_subdev;
> +	}
> +
>  	ret = v4l2_async_register_subdev(&sensor->sd);
>  	if (ret) {
>  		dev_err(dev, "Async subdev register failed: %d\n", ret);

-- 
Regards,
Benjamin