| Message ID | 20240720071606.27930-6-yunfei.dong@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: mediatek: add driver to support secure video decoder | expand |
On Sat, Jul 20, 2024 at 8:13 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > … > > +++ b/drivers/dma-buf/dma-heap.c > … > > +static void dma_heap_release(struct kref *ref) > > +{ > … > > + mutex_lock(&heap_list_lock); > > + list_del(&heap->list); > > + mutex_unlock(&heap_list_lock); > … > > Under which circumstances would you become interested to apply a statement > like “guard(mutex)(&heap_list_lock);”? > https://elixir.bootlin.com/linux/v6.10/source/include/linux/mutex.h#L196 This strikes me as a strange place to apply it, as it seems like it would grow the lock hold time to the entire scope of the function unless one created a subscope for just the list_del, at which point you're not saving much or really improving readability. I definitely think guard usage is very interesting in places where locks are released in multiple exit paths, etc. but this is a very trivial and straightforward lock/unlock usage, so I fret I don't quite understand the suggestion. thanks -john
>> … >>> +++ b/drivers/dma-buf/dma-heap.c >> … >>> +static void dma_heap_release(struct kref *ref) >>> +{ >> … >>> + mutex_lock(&heap_list_lock); >>> + list_del(&heap->list); >>> + mutex_unlock(&heap_list_lock); >> … >> >> Under which circumstances would you become interested to apply a statement >> like “guard(mutex)(&heap_list_lock);”? >> https://elixir.bootlin.com/linux/v6.10/source/include/linux/mutex.h#L196 > > This strikes me as a strange place to apply it, as it seems like it > would grow the lock hold time to the entire scope of the function > unless one created a subscope for just the list_del, at which point > you're not saving much or really improving readability. I definitely > think guard usage is very interesting in places where locks are > released in multiple exit paths, etc. but this is a very trivial and > straightforward lock/unlock usage, so I fret I don't quite understand > the suggestion. I propose to take further design possibilities better into account for applications of scope-based resource management. Additional compound statements may be constructed on demand by adding extra curly brackets. You might occasionally find scoped guards more appealing. https://elixir.bootlin.com/linux/v6.10/source/include/linux/cleanup.h#L137 Regards, Markus
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 22f6c193db0d..97025ee8500f 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,7 @@ * @heap_devt: heap device node * @list: list head connecting to list of heaps * @heap_cdev: heap char device + * @refcount: reference counter for this heap device * * Represents a heap of memory from which buffers can be made. */ @@ -40,6 +42,7 @@ struct dma_heap { dev_t heap_devt; struct list_head list; struct cdev heap_cdev; + struct kref refcount; }; static LIST_HEAD(heap_list); @@ -240,6 +243,7 @@ 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; @@ -304,6 +308,31 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) return err_ret; } +static void dma_heap_release(struct kref *ref) +{ + struct dma_heap *heap = container_of(ref, struct dma_heap, refcount); + unsigned int minor = MINOR(heap->heap_devt); + + 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, minor); + + kfree(heap); +} + +/** + * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it + * @heap: DMA-Heap whose reference count to decrement + */ +void dma_heap_put(struct dma_heap *heap) +{ + kref_put(&heap->refcount, dma_heap_release); +} + 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 fbe86ec889a8..d57593f8a1bc 100644 --- a/include/linux/dma-heap.h +++ b/include/linux/dma-heap.h @@ -46,4 +46,6 @@ const char *dma_heap_get_name(struct dma_heap *heap); struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info); +void dma_heap_put(struct dma_heap *heap); + #endif /* _DMA_HEAPS_H */