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

  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.

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

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

On Mon, Sep 11, 2023 at 2:49 AM Christian König
<christian.koenig@amd.com> wrote:
>
> 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.
>
Got it, thanks.

> > +
> > +     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.
>
Ok, I'll move this mutex acquisition to dma_heap_release so that it
guards just the list_del, and change dma_heap_find to use
kref_get_unless_zero. Thanks.

> > +     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.
>
Will fix.




> Regards,
> Christian.
>
> > +void dma_heap_put(struct dma_heap *heap);
> > +
> >   #endif /* _DMA_HEAPS_H */
>