[PATCH v3 0/3] Control handler free changes

[PATCH v3 0/3] Control handler free changes

[Sakari Ailus]

Hi folks,

This set changes the control handler's error code handing when the handler
is free and documents it.

since v3:

- Use ! for NULL checks, instead of comparing with NULL, in 2nd patch

since v2:

- Note in the 2nd patch v4l2_ctrl_handler_free() returns 0 if hdl is NULL, also
  avoid NULL pointer dereference in the same case.

- Only Documentation/ changes left in 3rd patch.

since v1:

- Added the Fixes: / Cc: stable tags.

- Added two patches to return the handler's error and document the error
  isn't changed when the handler is freed.

Sakari Ailus (3):
  media: v4l2-ctrls: Don't reset handler's error in
    v4l2_ctrl_handler_free()
  media: v4l2-ctrls: Return the handler's error in
    v4l2_ctrl_handler_free()
  media: Documentation: Document new v4l2_ctrl_handler_free() behaviour

 Documentation/driver-api/media/v4l2-controls.rst |  9 +++------
 drivers/media/v4l2-core/v4l2-ctrls-core.c        | 12 ++++++++----
 include/media/v4l2-ctrls.h                       |  4 +++-
 3 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.39.5

[PATCH v4 1/3] media: v4l2-ctrls: Don't reset handler's error in v4l2_ctrl_handler_free()

[Sakari Ailus]

It's a common pattern in drivers to free the control handler's resources
and then return the handler's error code on drivers' error handling paths.
Alas, the v4l2_ctrl_handler_free() function also zeroes the error field,
effectively indicating successful return to the caller.

There's no apparent need to touch the error field while releasing the
control handler's resources and cleaning up stale pointers. Not touching
the handler's error field is a more certain way to address this problem
than changing all the users, in which case the pattern would be likely to
re-emerge in new drivers.

Do just that, don't touch the control handler's error field in
v4l2_ctrl_handler_free().

Fixes: 0996517cf8ea ("V4L/DVB: v4l2: Add new control handling framework")
Cc: stable@vger.kernel.org
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-ctrls-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index b45809a82f9a..d28596c720d8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -1661,7 +1661,6 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 	kvfree(hdl->buckets);
 	hdl->buckets = NULL;
 	hdl->cached = NULL;
-	hdl->error = 0;
 	mutex_unlock(hdl->lock);
 	mutex_destroy(&hdl->_lock);
 }
-- 
2.39.5

[PATCH v4 2/3] media: v4l2-ctrls: Return the handler's error in v4l2_ctrl_handler_free()

[Sakari Ailus]

v4l2_ctrl_handler_free() used to return void but changing this to int,
returning the handler's error code, enables the drivers to simply return
the handler's error in this common error handling pattern:

	if (handler->error)
		return v4l2_ctrl_handler_free(handler);

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ctrls-core.c | 11 ++++++++---
 include/media/v4l2-ctrls.h                |  4 +++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index d28596c720d8..98b960775e87 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -1631,14 +1631,17 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
 EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
 
 /* Free all controls and control refs */
-void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
+int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 {
 	struct v4l2_ctrl_ref *ref, *next_ref;
 	struct v4l2_ctrl *ctrl, *next_ctrl;
 	struct v4l2_subscribed_event *sev, *next_sev;
 
-	if (hdl == NULL || hdl->buckets == NULL)
-		return;
+	if (!hdl)
+		return 0;
+
+	if (!hdl->buckets)
+		return hdl->error;
 
 	v4l2_ctrl_handler_free_request(hdl);
 
@@ -1663,6 +1666,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 	hdl->cached = NULL;
 	mutex_unlock(hdl->lock);
 	mutex_destroy(&hdl->_lock);
+
+	return hdl->error;
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_free);
 
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 3a87096e064f..c32c46286441 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -579,8 +579,10 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
  * @hdl:	The control handler.
  *
  * Does nothing if @hdl == NULL.
+ *
+ * Return: @hdl's error field or 0 if @hdl is NULL.
  */
-void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
+int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
 
 /**
  * v4l2_ctrl_lock() - Helper function to lock the handler
-- 
2.39.5

[PATCH v4 3/3] media: Documentation: Document new v4l2_ctrl_handler_free() behaviour

[Sakari Ailus]

v4l2_ctrl_handler_free() no longer resets the handler's error code.
Document it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/driver-api/media/v4l2-controls.rst | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/media/v4l2-controls.rst b/Documentation/driver-api/media/v4l2-controls.rst
index b2e91804829b..fc04907589ab 100644
--- a/Documentation/driver-api/media/v4l2-controls.rst
+++ b/Documentation/driver-api/media/v4l2-controls.rst
@@ -110,6 +110,7 @@ For sub-device drivers:
 
 	v4l2_ctrl_handler_free(&foo->ctrl_handler);
 
+:c:func:`v4l2_ctrl_handler_free` does not touch the handler's ``error`` field.
 
 2) Add controls:
 
@@ -191,12 +192,8 @@ These functions are typically called right after the
 			V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
 			0, test_pattern);
 	...
-	if (foo->ctrl_handler.error) {
-		int err = foo->ctrl_handler.error;
-
-		v4l2_ctrl_handler_free(&foo->ctrl_handler);
-		return err;
-	}
+	if (foo->ctrl_handler.error)
+		return v4l2_ctrl_handler_free(&foo->ctrl_handler);
 
 The :c:func:`v4l2_ctrl_new_std` function returns the v4l2_ctrl pointer to
 the new control, but if you do not need to access the pointer outside the
-- 
2.39.5

Re: [PATCH v4 1/3] media: v4l2-ctrls: Don't reset handler's error in v4l2_ctrl_handler_free()

[Laurent Pinchart]

On Mon, Jun 23, 2025 at 03:23:12PM +0300, Sakari Ailus wrote:
> It's a common pattern in drivers to free the control handler's resources
> and then return the handler's error code on drivers' error handling paths.
> Alas, the v4l2_ctrl_handler_free() function also zeroes the error field,
> effectively indicating successful return to the caller.
> 
> There's no apparent need to touch the error field while releasing the
> control handler's resources and cleaning up stale pointers. Not touching
> the handler's error field is a more certain way to address this problem
> than changing all the users, in which case the pattern would be likely to
> re-emerge in new drivers.
> 
> Do just that, don't touch the control handler's error field in
> v4l2_ctrl_handler_free().
> 
> Fixes: 0996517cf8ea ("V4L/DVB: v4l2: Add new control handling framework")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index b45809a82f9a..d28596c720d8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1661,7 +1661,6 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  	kvfree(hdl->buckets);
>  	hdl->buckets = NULL;
>  	hdl->cached = NULL;
> -	hdl->error = 0;
>  	mutex_unlock(hdl->lock);
>  	mutex_destroy(&hdl->_lock);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] media: v4l2-ctrls: Return the handler's error in v4l2_ctrl_handler_free()

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Mon, Jun 23, 2025 at 03:23:13PM +0300, Sakari Ailus wrote:
> v4l2_ctrl_handler_free() used to return void but changing this to int,
> returning the handler's error code, enables the drivers to simply return
> the handler's error in this common error handling pattern:
> 
> 	if (handler->error)
> 		return v4l2_ctrl_handler_free(handler);
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 11 ++++++++---
>  include/media/v4l2-ctrls.h                |  4 +++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index d28596c720d8..98b960775e87 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1631,14 +1631,17 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>  EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
>  
>  /* Free all controls and control refs */
> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  {
>  	struct v4l2_ctrl_ref *ref, *next_ref;
>  	struct v4l2_ctrl *ctrl, *next_ctrl;
>  	struct v4l2_subscribed_event *sev, *next_sev;
>  
> -	if (hdl == NULL || hdl->buckets == NULL)
> -		return;
> +	if (!hdl)
> +		return 0;
> +
> +	if (!hdl->buckets)
> +		return hdl->error;

!hdl->buckets means the handler has been freed, or is not initialized.
I'd return 0 in that case.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	v4l2_ctrl_handler_free_request(hdl);
>  
> @@ -1663,6 +1666,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  	hdl->cached = NULL;
>  	mutex_unlock(hdl->lock);
>  	mutex_destroy(&hdl->_lock);
> +
> +	return hdl->error;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_handler_free);
>  
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 3a87096e064f..c32c46286441 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -579,8 +579,10 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>   * @hdl:	The control handler.
>   *
>   * Does nothing if @hdl == NULL.
> + *
> + * Return: @hdl's error field or 0 if @hdl is NULL.
>   */
> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
>  
>  /**
>   * v4l2_ctrl_lock() - Helper function to lock the handler

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: Documentation: Document new v4l2_ctrl_handler_free() behaviour

[Laurent Pinchart]

On Mon, Jun 23, 2025 at 03:23:14PM +0300, Sakari Ailus wrote:
> v4l2_ctrl_handler_free() no longer resets the handler's error code.
> Document it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/driver-api/media/v4l2-controls.rst | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/driver-api/media/v4l2-controls.rst b/Documentation/driver-api/media/v4l2-controls.rst
> index b2e91804829b..fc04907589ab 100644
> --- a/Documentation/driver-api/media/v4l2-controls.rst
> +++ b/Documentation/driver-api/media/v4l2-controls.rst
> @@ -110,6 +110,7 @@ For sub-device drivers:
>  
>  	v4l2_ctrl_handler_free(&foo->ctrl_handler);
>  
> +:c:func:`v4l2_ctrl_handler_free` does not touch the handler's ``error`` field.
>  
>  2) Add controls:
>  
> @@ -191,12 +192,8 @@ These functions are typically called right after the
>  			V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
>  			0, test_pattern);
>  	...
> -	if (foo->ctrl_handler.error) {
> -		int err = foo->ctrl_handler.error;
> -
> -		v4l2_ctrl_handler_free(&foo->ctrl_handler);
> -		return err;
> -	}
> +	if (foo->ctrl_handler.error)
> +		return v4l2_ctrl_handler_free(&foo->ctrl_handler);
>  
>  The :c:func:`v4l2_ctrl_new_std` function returns the v4l2_ctrl pointer to
>  the new control, but if you do not need to access the pointer outside the

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] media: v4l2-ctrls: Return the handler's error in v4l2_ctrl_handler_free()

[Laurent Pinchart]

On Mon, Jun 23, 2025 at 03:27:19PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Mon, Jun 23, 2025 at 03:23:13PM +0300, Sakari Ailus wrote:
> > v4l2_ctrl_handler_free() used to return void but changing this to int,
> > returning the handler's error code, enables the drivers to simply return
> > the handler's error in this common error handling pattern:
> > 
> > 	if (handler->error)
> > 		return v4l2_ctrl_handler_free(handler);
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls-core.c | 11 ++++++++---
> >  include/media/v4l2-ctrls.h                |  4 +++-
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > index d28596c720d8..98b960775e87 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > @@ -1631,14 +1631,17 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
> >  EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
> >  
> >  /* Free all controls and control refs */
> > -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> > +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> >  {
> >  	struct v4l2_ctrl_ref *ref, *next_ref;
> >  	struct v4l2_ctrl *ctrl, *next_ctrl;
> >  	struct v4l2_subscribed_event *sev, *next_sev;
> >  
> > -	if (hdl == NULL || hdl->buckets == NULL)
> > -		return;
> > +	if (!hdl)
> > +		return 0;
> > +
> > +	if (!hdl->buckets)
> > +		return hdl->error;
> 
> !hdl->buckets means the handler has been freed, or is not initialized.
> I'd return 0 in that case.

Actually it can also mean init failed due to an allocation error, in
which case error is set to -ENOMEM. Please ignore this comment.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

You can keep this.

> >  
> >  	v4l2_ctrl_handler_free_request(hdl);
> >  
> > @@ -1663,6 +1666,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> >  	hdl->cached = NULL;
> >  	mutex_unlock(hdl->lock);
> >  	mutex_destroy(&hdl->_lock);
> > +
> > +	return hdl->error;
> >  }
> >  EXPORT_SYMBOL(v4l2_ctrl_handler_free);
> >  
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 3a87096e064f..c32c46286441 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -579,8 +579,10 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
> >   * @hdl:	The control handler.
> >   *
> >   * Does nothing if @hdl == NULL.
> > + *
> > + * Return: @hdl's error field or 0 if @hdl is NULL.
> >   */
> > -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
> > +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
> >  
> >  /**
> >   * v4l2_ctrl_lock() - Helper function to lock the handler

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] media: v4l2-ctrls: Return the handler's error in v4l2_ctrl_handler_free()

[Hans Verkuil]

On 23/06/2025 14:23, Sakari Ailus wrote:
> v4l2_ctrl_handler_free() used to return void but changing this to int,
> returning the handler's error code, enables the drivers to simply return
> the handler's error in this common error handling pattern:
> 
> 	if (handler->error)
> 		return v4l2_ctrl_handler_free(handler);
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 11 ++++++++---
>  include/media/v4l2-ctrls.h                |  4 +++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index d28596c720d8..98b960775e87 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1631,14 +1631,17 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>  EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
>  
>  /* Free all controls and control refs */
> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  {
>  	struct v4l2_ctrl_ref *ref, *next_ref;
>  	struct v4l2_ctrl *ctrl, *next_ctrl;
>  	struct v4l2_subscribed_event *sev, *next_sev;
>  
> -	if (hdl == NULL || hdl->buckets == NULL)
> -		return;
> +	if (!hdl)
> +		return 0;
> +
> +	if (!hdl->buckets)
> +		return hdl->error;
>  
>  	v4l2_ctrl_handler_free_request(hdl);
>  
> @@ -1663,6 +1666,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  	hdl->cached = NULL;
>  	mutex_unlock(hdl->lock);
>  	mutex_destroy(&hdl->_lock);
> +
> +	return hdl->error;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_handler_free);
>  
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 3a87096e064f..c32c46286441 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -579,8 +579,10 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>   * @hdl:	The control handler.
>   *
>   * Does nothing if @hdl == NULL.
> + *
> + * Return: @hdl's error field or 0 if @hdl is NULL.
>   */
> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
>  
>  /**
>   * v4l2_ctrl_lock() - Helper function to lock the handler

Re: [PATCH v4 2/3] media: v4l2-ctrls: Return the handler's error in v4l2_ctrl_handler_free()

[Hans Verkuil]

On 23/06/2025 14:27, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Mon, Jun 23, 2025 at 03:23:13PM +0300, Sakari Ailus wrote:
>> v4l2_ctrl_handler_free() used to return void but changing this to int,
>> returning the handler's error code, enables the drivers to simply return
>> the handler's error in this common error handling pattern:
>>
>> 	if (handler->error)
>> 		return v4l2_ctrl_handler_free(handler);
>>
>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 11 ++++++++---
>>  include/media/v4l2-ctrls.h                |  4 +++-
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> index d28596c720d8..98b960775e87 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> @@ -1631,14 +1631,17 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>>  EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
>>  
>>  /* Free all controls and control refs */
>> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>>  {
>>  	struct v4l2_ctrl_ref *ref, *next_ref;
>>  	struct v4l2_ctrl *ctrl, *next_ctrl;
>>  	struct v4l2_subscribed_event *sev, *next_sev;
>>  
>> -	if (hdl == NULL || hdl->buckets == NULL)
>> -		return;
>> +	if (!hdl)
>> +		return 0;
>> +
>> +	if (!hdl->buckets)
>> +		return hdl->error;
> 
> !hdl->buckets means the handler has been freed, or is not initialized.
> I'd return 0 in that case.

Ah, no. If v4l2_ctrl_handler_init is called, and allocating hdl->buckets fails,
then hdl->error is set to -ENOMEM. You can just continue adding controls etc. and
do this at the end:

	if (foo->ctrl_handler.error)
		return v4l2_ctrl_handler_free(&foo->ctrl_handler);

You want to return -ENOMEM in that case, not 0.

Regards,

	Hans

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>  
>>  	v4l2_ctrl_handler_free_request(hdl);
>>  
>> @@ -1663,6 +1666,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>>  	hdl->cached = NULL;
>>  	mutex_unlock(hdl->lock);
>>  	mutex_destroy(&hdl->_lock);
>> +
>> +	return hdl->error;
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_handler_free);
>>  
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index 3a87096e064f..c32c46286441 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -579,8 +579,10 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
>>   * @hdl:	The control handler.
>>   *
>>   * Does nothing if @hdl == NULL.
>> + *
>> + * Return: @hdl's error field or 0 if @hdl is NULL.
>>   */
>> -void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
>> +int v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
>>  
>>  /**
>>   * v4l2_ctrl_lock() - Helper function to lock the handler
> 

Re: [PATCH v4 3/3] media: Documentation: Document new v4l2_ctrl_handler_free() behaviour

[Hans Verkuil]

On 23/06/2025 14:23, Sakari Ailus wrote:
> v4l2_ctrl_handler_free() no longer resets the handler's error code.
> Document it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> ---
>  Documentation/driver-api/media/v4l2-controls.rst | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/driver-api/media/v4l2-controls.rst b/Documentation/driver-api/media/v4l2-controls.rst
> index b2e91804829b..fc04907589ab 100644
> --- a/Documentation/driver-api/media/v4l2-controls.rst
> +++ b/Documentation/driver-api/media/v4l2-controls.rst
> @@ -110,6 +110,7 @@ For sub-device drivers:
>  
>  	v4l2_ctrl_handler_free(&foo->ctrl_handler);
>  
> +:c:func:`v4l2_ctrl_handler_free` does not touch the handler's ``error`` field.
>  
>  2) Add controls:
>  
> @@ -191,12 +192,8 @@ These functions are typically called right after the
>  			V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
>  			0, test_pattern);
>  	...
> -	if (foo->ctrl_handler.error) {
> -		int err = foo->ctrl_handler.error;
> -
> -		v4l2_ctrl_handler_free(&foo->ctrl_handler);
> -		return err;
> -	}
> +	if (foo->ctrl_handler.error)
> +		return v4l2_ctrl_handler_free(&foo->ctrl_handler);
>  
>  The :c:func:`v4l2_ctrl_new_std` function returns the v4l2_ctrl pointer to
>  the new control, but if you do not need to access the pointer outside the