Re: [PATCH 2/9] dma-heap: Add proper kref handling on dma-buf heaps

["Christian König" <christian.koenig@amd.com>]

Am 11.09.23 um 04:30 schrieb Yong Wu:
> From: John Stultz <jstultz@google.com>
>
> Add proper refcounting on the dma_heap structure.
> While existing heaps are built-in, we may eventually
> have heaps loaded from modules, and we'll need to be
> able to properly handle the references to the heaps
>
> Also moves minor tracking into the heap structure so
> we can properly free things.

This is completely unnecessary, see below.

>
> Signed-off-by: John Stultz <jstultz@google.com>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Just add comment for "minor" and "refcount"]
> ---
>   drivers/dma-buf/dma-heap.c | 38 ++++++++++++++++++++++++++++++++++----
>   include/linux/dma-heap.h   |  6 ++++++
>   2 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 51030f6c9d6e..dcc0e38c61fa 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -11,6 +11,7 @@
>   #include <linux/dma-buf.h>
>   #include <linux/dma-heap.h>
>   #include <linux/err.h>
> +#include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/nospec.h>
>   #include <linux/syscalls.h>
> @@ -30,6 +31,8 @@
>    * @heap_devt:		heap device node
>    * @list:		list head connecting to list of heaps
>    * @heap_cdev:		heap char device
> + * @minor:		heap device node minor number
> + * @refcount:		reference counter for this heap device
>    *
>    * Represents a heap of memory from which buffers can be made.
>    */
> @@ -40,6 +43,8 @@ struct dma_heap {
>   	dev_t heap_devt;
>   	struct list_head list;
>   	struct cdev heap_cdev;
> +	int minor;
> +	struct kref refcount;
>   };
>   
>   static LIST_HEAD(heap_list);
> @@ -205,7 +210,6 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   {
>   	struct dma_heap *heap, *h, *err_ret;
>   	struct device *dev_ret;
> -	unsigned int minor;
>   	int ret;
>   
>   	if (!exp_info->name || !strcmp(exp_info->name, "")) {
> @@ -222,12 +226,13 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	if (!heap)
>   		return ERR_PTR(-ENOMEM);
>   
> +	kref_init(&heap->refcount);
>   	heap->name = exp_info->name;
>   	heap->ops = exp_info->ops;
>   	heap->priv = exp_info->priv;
>   
>   	/* Find unused minor number */
> -	ret = xa_alloc(&dma_heap_minors, &minor, heap,
> +	ret = xa_alloc(&dma_heap_minors, &heap->minor, heap,
>   		       XA_LIMIT(0, NUM_HEAP_MINORS - 1), GFP_KERNEL);
>   	if (ret < 0) {
>   		pr_err("dma_heap: Unable to get minor number for heap\n");
> @@ -236,7 +241,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	}
>   
>   	/* Create device */
> -	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), minor);
> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
>   
>   	cdev_init(&heap->heap_cdev, &dma_heap_fops);
>   	ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1);
> @@ -280,12 +285,37 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   err2:
>   	cdev_del(&heap->heap_cdev);
>   err1:
> -	xa_erase(&dma_heap_minors, minor);
> +	xa_erase(&dma_heap_minors, heap->minor);
>   err0:
>   	kfree(heap);
>   	return err_ret;
>   }
>   
> +static void dma_heap_release(struct kref *ref)
> +{
> +	struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
> +
> +	/* Note, we already holding the heap_list_lock here */
> +	list_del(&heap->list);
> +
> +	device_destroy(dma_heap_class, heap->heap_devt);
> +	cdev_del(&heap->heap_cdev);
> +	xa_erase(&dma_heap_minors, heap->minor);

You can just use MINOR(heap->heap_devt) here instead.

> +
> +	kfree(heap);
> +}
> +
> +void dma_heap_put(struct dma_heap *h)
> +{
> +	/*
> +	 * Take the heap_list_lock now to avoid racing with code
> +	 * scanning the list and then taking a kref.
> +	 */

This is usually considered a bad idea since it makes the kref approach 
superfluous.

There are multiple possibilities how handle this, the most common one is 
to use kref_get_unless_zero() in your list traversal code and ignore the 
entry when that fails.

Alternatively you could use kref_put_mutex() instead. This gives you the 
same functionality as this here, but as far as I know it's normally only 
used in a couple of special cases.

> +	mutex_lock(&heap_list_lock);
> +	kref_put(&h->refcount, dma_heap_release);
> +	mutex_unlock(&heap_list_lock);
> +}
> +
>   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
>   {
>   	return kasprintf(GFP_KERNEL, "dma_heap/%s", dev_name(dev));
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index c7c29b724ad6..f3c678892c5c 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -64,4 +64,10 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>    */
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>   
> +/**
> + * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
> + * @heap: the heap whose reference count to decrement
> + */

Please don't add kerneldoc to the definition, add it to the 
implementation of the function.

Regards,
Christian.

> +void dma_heap_put(struct dma_heap *heap);
> +
>   #endif /* _DMA_HEAPS_H */