Re: [PATCH v2] media: i2c: vd56g3: clean up subdev state on probe failure

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Benjamin,

On Tue, Jun 16, 2026 at 02:49:54PM +0200, Benjamin Mugnier wrote:
> Hi,
> 
> Thank you for your patch, and apologies for the delay.
> 
> Le 24/04/2026 à 18:52, 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.
> > 
> > Route this error through a subdev cleanup label before freeing the control
> > handler and media entity.
> > 
> > Fixes: 87aa97fc3157 ("media: i2c: Add driver for ST VD56G3 camera sensor")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> > ---
> > Changes in v2:
> > - Use a lowercase subject summary.
> 
> Please keep the first character uppercase, just like other commits on
> this module.
> 
> > 
> >  drivers/media/i2c/vd56g3.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/vd56g3.c b/drivers/media/i2c/vd56g3.c
> > index 157acea9e2..43f792288a 100644
> > --- a/drivers/media/i2c/vd56g3.c
> > +++ b/drivers/media/i2c/vd56g3.c
> > @@ -1427,11 +1427,14 @@ static int vd56g3_subdev_init(struct vd56g3 *sensor)
> >  	v4l2_subdev_unlock_state(state);
> >  	if (ret) {
> >  		dev_err(sensor->dev, "Controls update failed: %d\n", ret);
> > -		goto err_ctrls;
> > +		goto err_subdev;
> >  	}
> >  
> >  	return 0;
> >  
> > +err_subdev:
> > +	v4l2_subdev_cleanup(&sensor->sd);
> 
> v4l2_subdev_cleanup() is already performed in the caller (i.e.
> vd56g3_probe()), but as you noticed it is not called from this path. I'd
> rather have the return value route correctly through
> v4l2_subdev_cleanup() in  vd56g3_probe(), allowing to keep a unique call
> to v4l2_subdev_cleanup() instead.

Is it?

If vd56g3_update_controls() in vd56g3_subdev_init() fails, it'll jump to
err_power_off in vd56g3_probe() which does PM related cleanup only.

> 
> This patch looks like is LLM generated and sparks my curiosity. If so
> you must disclaim it using an Assisted-by tag [1]. Sorry if I’m mistaken.
> 
> [1] https://docs.kernel.org/process/coding-assistants.html
> 
> > +
> >  err_ctrls:
> >  	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> >  
> 
> -- 
> Regards,
> Benjamin
> 

-- 
Kind regards,

Sakari Ailus