Hi John,
On Sat, Jul 25, 2020 at 03:26:33AM +0000, John Stultz wrote:
> 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
I'm not 100% clear on the intention here. What would take/drop a
reference on a heap?
In the case of modules I think the bigger problem is how to prevent
the module getting removed while there's still something using it.
Cheers,
-Brian
>
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Cc: Andrew F. Davis <afd@xxxxxx>
> Cc: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> Cc: Liam Mark <lmark@xxxxxxxxxxxxxx>
> Cc: Laura Abbott <labbott@xxxxxxxxxx>
> Cc: Brian Starkey <Brian.Starkey@xxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> drivers/dma-buf/dma-heap.c | 31 +++++++++++++++++++++++++++----
> include/linux/dma-heap.h | 6 ++++++
> 2 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index afd22c9dbdcf..90c3720acc1c 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -40,6 +40,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);
> @@ -190,11 +192,31 @@ void *dma_heap_get_drvdata(struct dma_heap *heap)
> return heap->priv;
> }
>
> +static void dma_heap_release(struct kref *ref)
> +{
> + struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
> +
> + /* Remove heap from the list */
> + mutex_lock(&heap_list_lock);
> + list_del(&heap->list);
> + mutex_unlock(&heap_list_lock);
> +
> + 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)
> +{
> + kref_put(&h->refcount, dma_heap_release);
> +}
> +
> 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, "")) {
> @@ -223,12 +245,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");
> @@ -237,7 +260,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);
> @@ -267,7 +290,7 @@ 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;
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index 454e354d1ffb..c1572f29cfac 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -56,4 +56,10 @@ void *dma_heap_get_drvdata(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 heaps, potentially freeing it
> + * @heap: heap pointer
> + */
> +void dma_heap_put(struct dma_heap *heap);
> +
> #endif /* _DMA_HEAPS_H */
> --
> 2.17.1
>
