On Mon, Sep 11, 2023 at 2:49 AM Christian König <christian.koenig@xxxxxxx> wrote: > > Am 11.09.23 um 04:30 schrieb Yong Wu: > > From: John Stultz <jstultz@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Signed-off-by: T.J. Mercier <tjmercier@xxxxxxxxxx> > > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx> > > [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 */ >