| Message ID | 20230911023038.30649-3-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: heaps: Add MediaTek secure heap | expand |
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 */ >
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); + + 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. + */ + 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 + */ +void dma_heap_put(struct dma_heap *heap); + #endif /* _DMA_HEAPS_H */