[PATCH 1/2] list: Add list_is_null() to check if a list_head has been initialized

[PATCH 1/2] list: Add list_is_null() to check if a list_head has been initialized

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The new function checks if the list_head prev and next pointers are
NULL, in order to see if a list_head that has been zeroed when allocated
has been initialized with INIT_LIST_HEAD() or added to a list.

This can be used in cleanup functions that want to support being safely
called when an object has not been initialized, to return immediately.
In most cases other fields of the object can be checked for this
purpose, but in some cases a list_head field is the only option.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 include/linux/list.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/list.h b/include/linux/list.h
index 85c92555e31f..e4fc6954de3b 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -29,6 +29,19 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
 	list->prev = list;
 }
 
+/**
+ * list_is_null - check if a list_head has been initialized
+ * @list: the list
+ *
+ * Check if the list_head prev and next pointers are NULL. This is useful to
+ * see if a list_head that has been zeroed when allocated has been initialized
+ * with INIT_LIST_HEAD() or added to a list.
+ */
+static inline bool list_is_null(struct list_head *list)
+{
+	return list->prev == NULL && list->next == NULL;
+}
+
 #ifdef CONFIG_DEBUG_LIST
 extern bool __list_add_valid(struct list_head *new,
 			      struct list_head *prev,
-- 
Regards,

Laurent Pinchart

[PATCH 2/2] media: v4l2-async: Safely unregister an non-registered async subdev

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Make the V4L2 async framework a bit more robust by allowing to
unregister a non-registered async subdev. Otherwise the
v4l2_async_cleanup() will attempt to delete the async subdev from the
subdev_list with the corresponding list_head not initialized.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 37cc0263b273..2347b7ac54d4 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
 
 void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 {
+	if (list_is_null(&sd->async_list))
+		return;
+
 	mutex_lock(&list_lock);
 
 	__v4l2_async_notifier_unregister(sd->subdev_notifier);
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] list: Add list_is_null() to check if a list_head has been initialized

[Sergei Shtylyov]

Hello!

On 27.02.2021 1:49, Laurent Pinchart wrote:

> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> The new function checks if the list_head prev and next pointers are
> NULL, in order to see if a list_head that has been zeroed when allocated
> has been initialized with INIT_LIST_HEAD() or added to a list.

    So zeroed or initialized/added? :-)

> This can be used in cleanup functions that want to support being safely
> called when an object has not been initialized, to return immediately.
> In most cases other fields of the object can be checked for this
> purpose, but in some cases a list_head field is the only option.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   include/linux/list.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/list.h b/include/linux/list.h
> index 85c92555e31f..e4fc6954de3b 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -29,6 +29,19 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
>   	list->prev = list;
>   }
>   
> +/**
> + * list_is_null - check if a list_head has been initialized
> + * @list: the list
> + *
> + * Check if the list_head prev and next pointers are NULL. This is useful to
> + * see if a list_head that has been zeroed when allocated has been initialized
> + * with INIT_LIST_HEAD() or added to a list.

    So zeroed or initialized/added? :-)

> + */
> +static inline bool list_is_null(struct list_head *list)
> +{
> +	return list->prev == NULL && list->next == NULL;

    Maybe instead:

	return !list->prev && !list->next;

[...]

MBR, Sergei

Re: [PATCH 1/2] list: Add list_is_null() to check if a list_head has been initialized

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Sat, Feb 27, 2021 at 12:49:37AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> The new function checks if the list_head prev and next pointers are
> NULL, in order to see if a list_head that has been zeroed when allocated
> has been initialized with INIT_LIST_HEAD() or added to a list.
> 
> This can be used in cleanup functions that want to support being safely
> called when an object has not been initialized, to return immediately.
> In most cases other fields of the object can be checked for this
> purpose, but in some cases a list_head field is the only option.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  include/linux/list.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/list.h b/include/linux/list.h
> index 85c92555e31f..e4fc6954de3b 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -29,6 +29,19 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
>  	list->prev = list;
>  }
>  
> +/**
> + * list_is_null - check if a list_head has been initialized
> + * @list: the list
> + *
> + * Check if the list_head prev and next pointers are NULL. This is useful to
> + * see if a list_head that has been zeroed when allocated has been initialized
> + * with INIT_LIST_HEAD() or added to a list.

How this should work with an entry that has been removed from a list with
list_del()? The values will be LIST_POISON[12] and so this function will
return true. Should it return false instead?

> + */
> +static inline bool list_is_null(struct list_head *list)
> +{
> +	return list->prev == NULL && list->next == NULL;

What would you think of issuing a warning if one is NULL but the other one
isn't? That could happen if the memory is uninitialised by the caller. It
should return true in that case, too.

> +}
> +
>  #ifdef CONFIG_DEBUG_LIST
>  extern bool __list_add_valid(struct list_head *new,
>  			      struct list_head *prev,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: v4l2-async: Safely unregister an non-registered async subdev

[Sakari Ailus]

Hi Laurent,

On Sat, Feb 27, 2021 at 12:49:38AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Make the V4L2 async framework a bit more robust by allowing to
> unregister a non-registered async subdev. Otherwise the
> v4l2_async_cleanup() will attempt to delete the async subdev from the
> subdev_list with the corresponding list_head not initialized.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

IMO this can be merged through another tree once the first patch is agreed
on. Cc Hans and Mauro, too.

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 37cc0263b273..2347b7ac54d4 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
>  
>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  {
> +	if (list_is_null(&sd->async_list))
> +		return;
> +
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_notifier_unregister(sd->subdev_notifier);

-- 
Kind regards,

Sakari Ailus