Am 22.12.21 um 22:21 schrieb Daniel Vetter:
On Tue, Dec 07, 2021 at 01:33:51PM +0100, Christian König wrote:
Add a function to simplify getting a single fence for all the fences in
the dma_resv object.
v2: fix ref leak in error handling
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
drivers/dma-buf/dma-resv.c | 52 ++++++++++++++++++++++++++++++++++++++
include/linux/dma-resv.h | 2 ++
2 files changed, 54 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 480c305554a1..694716a3d66d 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -34,6 +34,7 @@
*/
#include <linux/dma-resv.h>
+#include <linux/dma-fence-array.h>
#include <linux/export.h>
#include <linux/mm.h>
#include <linux/sched/mm.h>
@@ -657,6 +658,57 @@ int dma_resv_get_fences(struct dma_resv *obj, bool write,
}
EXPORT_SYMBOL_GPL(dma_resv_get_fences);
+/**
+ * dma_resv_get_singleton - Get a single fence for all the fences
+ * @obj: the reservation object
+ * @write: true if we should return all fences
+ * @fence: the resulting fence
+ *
+ * Get a single fence representing all the fences inside the resv object.
+ * Returns either 0 for success or -ENOMEM.
+ *
+ * Warning: This can't be used like this when adding the fence back to the resv
+ * object since that can lead to stack corruption when finalizing the
+ * dma_fence_array.
Uh I don't get this one? I thought the only problem with nested fences is
the signalling recursion, which we work around with the irq_work?
Nope, the main problem is finalizing the dma_fence_array.
E.g. imagine that you build up a chain of dma_fence_array objects like this:
a<-b<-c<-d<-e<-f.....
With each one referencing the previous dma_fence_array and then you call
dma_fence_put() on the last one. That in turn will cause calling
dma_fence_put() on the previous one, which in turn will cause
dma_fence_put() one the one before the previous one etc....
In other words you recurse because each dma_fence_array instance drops
the last reference of it's predecessor.
What we could do is to delegate dropping the reference to the containing
fences in a dma_fence_array as well, but that would require some changes
to the irq_work_run_list() function to be halve way efficient.
Also if there's really an issue with dma_fence_array fences, then that
warning should be on the dma_resv kerneldoc, not somewhere hidden like
this. And finally I really don't see what can go wrong, sure we'll end up
with the same fence once in the dma_resv_list and then once more in the
fence array. But they're all refcounted, so really shouldn't matter.
The code itself looks correct, but me not understanding what even goes
wrong here freaks me out a bit.
Yeah, IIRC we already discussed that with Jason in length as well.
Essentially what you can't do is to put a dma_fence_array into another
dma_fence_array without causing issues.
So I think we should maybe just add a WARN_ON() into
dma_fence_array_init() to make sure that this never happens.
Regards,
Christian.
I guess something to figure out next year, I kinda hoped I could squeeze a
review in before I disappear :-/
-Daniel
+ */
+int dma_resv_get_singleton(struct dma_resv *obj, bool write,
+ struct dma_fence **fence)
+{
+ struct dma_fence_array *array;
+ struct dma_fence **fences;
+ unsigned count;
+ int r;
+
+ r = dma_resv_get_fences(obj, write, &count, &fences);
+ if (r)
+ return r;
+
+ if (count == 0) {
+ *fence = NULL;
+ return 0;
+ }
+
+ if (count == 1) {
+ *fence = fences[0];
+ kfree(fences);
+ return 0;
+ }
+
+ array = dma_fence_array_create(count, fences,
+ dma_fence_context_alloc(1),
+ 1, false);
+ if (!array) {
+ while (count--)
+ dma_fence_put(fences[count]);
+ kfree(fences);
+ return -ENOMEM;
+ }
+
+ *fence = &array->base;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dma_resv_get_singleton);
+
/**
* dma_resv_wait_timeout - Wait on reservation's objects
* shared and/or exclusive fences.
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
index fa2002939b19..cdfbbda6f600 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -438,6 +438,8 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence);
int dma_resv_get_fences(struct dma_resv *obj, bool write,
unsigned int *num_fences, struct dma_fence ***fences);
+int dma_resv_get_singleton(struct dma_resv *obj, bool write,
+ struct dma_fence **fence);
int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr,
unsigned long timeout);
--
2.25.1
