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

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

[Myeonghun Pak]

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.

 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);
+
 err_ctrls:
 	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
 
-- 
2.50.1

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

[Benjamin Mugnier]

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.

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

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

[Sakari Ailus]

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

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

[Benjamin Mugnier]

Hi Sakari,

Le 18/06/2026 à 13:53, Sakari Ailus a écrit :
> 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.
> 

Exactly, I realize my sentence was poorly written, but if I understand
correctly we're on the same page. The problem being
v4l2_subdev_cleanup() not being called in any path if
vd56g3_update_controls() fails.

Now if vd56g3_update_controls() fails, instead of performing
v4l2_subdev_cleanup() in vd56g3_subdev_init() as this patch does, I'd
rather have it done in vd56g3_probe()'s jump back so we can keep it all
at the same place, instead of having 2 v4l2_subdev_cleanup() in 2
different places.

Tell me if this is still unclear.

>>
>> 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
>>
> 

-- 
Regards,
Benjamin

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

[Myeonghun Pak]

Hi Benjamin, Sakari,

Thank you for the clarification.

I agree. The missing cleanup is on the vd56g3_update_controls() failure
path after v4l2_subdev_init_finalize() has succeeded.

I'll send a v3 that keeps v4l2_subdev_cleanup() in
vd56g3_subdev_cleanup(), and routes this failure through the probe-side
err_subdev path instead of adding another v4l2_subdev_cleanup() call in
vd56g3_subdev_init().

I'll also add the appropriate Assisted-by tag.

Regards,
Myeonghun

2026년 6월 19일 (금) 오후 5:59, Benjamin Mugnier <benjamin.mugnier@foss.st.com>님이 작성:
>
> Hi Sakari,
>
> Le 18/06/2026 à 13:53, Sakari Ailus a écrit :
> > 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.
> >
>
> Exactly, I realize my sentence was poorly written, but if I understand
> correctly we're on the same page. The problem being
> v4l2_subdev_cleanup() not being called in any path if
> vd56g3_update_controls() fails.
>
> Now if vd56g3_update_controls() fails, instead of performing
> v4l2_subdev_cleanup() in vd56g3_subdev_init() as this patch does, I'd
> rather have it done in vd56g3_probe()'s jump back so we can keep it all
> at the same place, instead of having 2 v4l2_subdev_cleanup() in 2
> different places.
>
> Tell me if this is still unclear.
>
> >>
> >> 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
> >>
> >
>
> --
> Regards,
> Benjamin
>