Re: [PATCH 16/55] media: v4l2-async: Add notifier operation to destroy asd instances

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Paul Elder <paul.elder@ideasonboard.com>
Cc: linux-media@vger.kernel.org, dafna@fastmail.com, heiko@sntech.de,
	jeanmichel.hautbois@ideasonboard.com, jacopo@jmondi.org,
	djrscally@gmail.com, helen.koike@collabora.com,
	linux-rockchip@lists.infradead.org,
	Sakari Ailus <sakari.ailus@iki.fi>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>
Subject: Re: [PATCH 16/55] media: v4l2-async: Add notifier operation to destroy asd instances
Date: Thu, 16 Jun 2022 01:36:09 +0300	[thread overview]
Message-ID: <Yqpe2e8WzeDkAhYj@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220614191127.3420492-17-paul.elder@ideasonboard.com>

+ Sakari and Hans

This patch is needed by this series, but I think it could also be
reviewed and merged standalone already.

On Wed, Jun 15, 2022 at 04:10:48AM +0900, Paul Elder wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Drivers typically extend the v4l2_async_subdev structure by embedding it
> in a driver-specific structure, to store per-subdev custom data. The
> v4l2_async_subdev instances are freed by the v4l2-async framework, which
> makes this mechanism cumbersome to use safely when custom data needs
> special treatment to be destroyed (such as freeing additional memory, or
> releasing references to kernel objects).
> 
> To ease this, add a .destroy() operation to the
> v4l2_async_notifier_operations structure. The operation is called right
> before the v4l2_async_subdev is freed, giving drivers a chance to
> destroy data if needed.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/driver-api/media/v4l2-subdev.rst |  6 ++++++
>  drivers/media/v4l2-core/v4l2-async.c           | 10 ++++++++++
>  include/media/v4l2-async.h                     |  2 ++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index cf3b52bdbfb9..6f8d79926aa5 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -243,6 +243,12 @@ notifier callback is called. After all subdevices have been located the
>  .complete() callback is called. When a subdevice is removed from the
>  system the .unbind() method is called. All three callbacks are optional.
>  
> +Drivers can store any type of custom data in their driver-specific
> +:c:type:`v4l2_async_subdev` wrapper. If any of that data requires special
> +handling when the structure is freed, drivers must implement the ``.destroy()``
> +notifier callback. The framework will call it right before freeing the
> +:c:type:`v4l2_async_subdev`.
> +
>  Calling subdev operations
>  ~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index c6995718237a..735dede624b8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -52,6 +52,15 @@ static int v4l2_async_nf_call_complete(struct v4l2_async_notifier *n)
>  	return n->ops->complete(n);
>  }
>  
> +static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n,
> +				       struct v4l2_async_subdev *asd)
> +{
> +	if (!n->ops || !n->ops->destroy)
> +		return;
> +
> +	n->ops->destroy(asd);
> +}
> +
>  static bool match_i2c(struct v4l2_async_notifier *notifier,
>  		      struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> @@ -626,6 +635,7 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier)
>  		}
>  
>  		list_del(&asd->asd_list);
> +		v4l2_async_nf_call_destroy(notifier, asd);
>  		kfree(asd);
>  	}
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 13ff3ad948f4..25eb1d138c06 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -81,6 +81,7 @@ struct v4l2_async_subdev {
>   * @complete:	All subdevices have been probed successfully. The complete
>   *		callback is only executed for the root notifier.
>   * @unbind:	a subdevice is leaving
> + * @destroy:	the asd is about to be freed
>   */
>  struct v4l2_async_notifier_operations {
>  	int (*bound)(struct v4l2_async_notifier *notifier,
> @@ -90,6 +91,7 @@ struct v4l2_async_notifier_operations {
>  	void (*unbind)(struct v4l2_async_notifier *notifier,
>  		       struct v4l2_subdev *subdev,
>  		       struct v4l2_async_subdev *asd);
> +	void (*destroy)(struct v4l2_async_subdev *asd);
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart