This callback is pretty much deprecated and should not be used by new implementations.
Clarify that in the documentation as well.
Signed-off-by: Christian König christian.koenig@amd.com --- include/linux/dma-fence.h | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 6ffb4b2c6371..a44e42b86c2a 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -214,19 +214,15 @@ struct dma_fence_ops { * Custom wait implementation, defaults to dma_fence_default_wait() if * not set. * - * The dma_fence_default_wait implementation should work for any fence, as long - * as @enable_signaling works correctly. This hook allows drivers to - * have an optimized version for the case where a process context is - * already available, e.g. if @enable_signaling for the general case - * needs to set up a worker thread. + * Deprecated and should not be used by new implementations. Only used + * by existing implementations which need special handling for their + * hardware reset procedure. * * Must return -ERESTARTSYS if the wait is intr = true and the wait was * interrupted, and remaining jiffies if fence has signaled, or 0 if wait * timed out. Can also return other error values on custom implementations, * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that. - * - * This callback is optional. */ signed long (*wait)(struct dma_fence *fence, bool intr, signed long timeout);
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * @cb: the callback to register * @func: the function to call * + * Add a software callback to the fence. The caller should keep a reference to + * the fence. + * * @cb will be initialized by dma_fence_add_callback(), no initialization * by the caller is required. Any number of callbacks can be registered * to a fence, but a callback can only be registered to one fence at a time. * - * Note that the callback can be called from an atomic context. If - * fence is already signaled, this function will return -ENOENT (and + * If fence is already signaled, this function will return -ENOENT (and * *not* call the callback). * - * Add a software callback to the fence. Same restrictions apply to - * refcount as it does to dma_fence_wait(), however the caller doesn't need to - * keep a refcount to fence afterward dma_fence_add_callback() has returned: - * when software access is enabled, the creator of the fence is required to keep - * the fence alive until after it signals with dma_fence_signal(). The callback - * itself can be called from irq context. + * Note that the callback can be called from an atomic context or irq context. * * Returns 0 in case of success, -ENOENT if the fence is already signaled * and -EINVAL in case of error.
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
- @cb: the callback to register
- @func: the function to call
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence. But since the fence can disappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
Am 21.07.21 um 13:52 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
Yeah, I'm seeing the problems but I'm not really good at documenting things either.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
- @cb: the callback to register
- @func: the function to call
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence.
I'm not sure that is a good wording since it can be misinterpreted once more.
But since the fence can disappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Ok if you want to avoid "should" then I would rather write:
The caller must make sure that there is a reference to the fence until the callback is called or removed.
Christian.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
On Wed, Jul 21, 2021 at 3:18 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 13:52 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
Yeah, I'm seeing the problems but I'm not really good at documenting things either.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
- @cb: the callback to register
- @func: the function to call
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence.
I'm not sure that is a good wording since it can be misinterpreted once more.
But since the fence can disappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Ok if you want to avoid "should" then I would rather write:
The caller must make sure that there is a reference to the fence until the callback is called or removed.
Yeah but is that really the case? If you never remove the callback yourself and instead just wait until the cb is called, then that should be safe? Assuming you don't look at the fence afterwards at all. It's just that in practice there's tons of reasons why you might need to bail out and remove the cb, and at that point you can race and need your own reference, or things go boom.
So there's no unconditional requirement to hold a reference. -Daniel
Christian.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
Am 21.07.21 um 15:36 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 3:18 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 13:52 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
Yeah, I'm seeing the problems but I'm not really good at documenting things either.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * @cb: the callback to register * @func: the function to call *
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence.
I'm not sure that is a good wording since it can be misinterpreted once more.
But since the fence can disappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Ok if you want to avoid "should" then I would rather write:
The caller must make sure that there is a reference to the fence until the callback is called or removed.
Yeah but is that really the case? If you never remove the callback yourself and instead just wait until the cb is called, then that should be safe? Assuming you don't look at the fence afterwards at all. It's just that in practice there's tons of reasons why you might need to bail out and remove the cb, and at that point you can race and need your own reference, or things go boom.
So there's no unconditional requirement to hold a reference.
Yeah and exactly because of this I want to document that you *must* keep a reference around because people tend to get this stuff wrong if you are not strict about it and it works in some cases but not others.
Christian.
-Daniel
Christian.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
On Wed, Jul 21, 2021 at 3:57 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 15:36 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 3:18 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 13:52 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
Yeah, I'm seeing the problems but I'm not really good at documenting things either.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * @cb: the callback to register * @func: the function to call *
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence.
I'm not sure that is a good wording since it can be misinterpreted once more.
But since the fence can disappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Ok if you want to avoid "should" then I would rather write:
The caller must make sure that there is a reference to the fence until the callback is called or removed.
Yeah but is that really the case? If you never remove the callback yourself and instead just wait until the cb is called, then that should be safe? Assuming you don't look at the fence afterwards at all. It's just that in practice there's tons of reasons why you might need to bail out and remove the cb, and at that point you can race and need your own reference, or things go boom.
So there's no unconditional requirement to hold a reference.
Yeah and exactly because of this I want to document that you *must* keep a reference around because people tend to get this stuff wrong if you are not strict about it and it works in some cases but not others.
Well I think docs should explain why/when you must hold a reference, like "must hold a reference if", but also explain that the call itself doesn't really require it's own reference that you need to drop in the callback. Hence the distinction of what's strictly needed, and what's needed in most practical cases. -Daniel
Christian.
-Daniel
Christian.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
Am 21.07.21 um 16:37 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 3:57 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 15:36 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 3:18 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 21.07.21 um 13:52 schrieb Daniel Vetter:
On Wed, Jul 21, 2021 at 11:21:33AM +0200, Christian König wrote:
That the caller doesn't need to keep a reference is rather risky and not defensive at all.
Especially dma_buf_poll got that horrible wrong, so better remove that sentence and also clarify that the callback might be called in atomic or interrupt context.
Signed-off-by: Christian König christian.koenig@amd.com
I'm very vary of aspirational interface docs for cross-anything, it just means everyone does whatever they feel like. I think I get what you aim for here, but this needs more careful wording.
Yeah, I'm seeing the problems but I'm not really good at documenting things either.
drivers/dma-buf/dma-fence.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ce0f5eff575d..1e82ecd443fa 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -616,20 +616,17 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * @cb: the callback to register * @func: the function to call *
- Add a software callback to the fence. The caller should keep a reference to
- the fence.
Instead of your "The caller should" what about:
It is not required to hold rerence to @fence.
I'm not sure that is a good wording since it can be misinterpreted once more.
But since the fence candisappear as soon as @cb has returned callers generally must hold their own reference to prevent issues.
With that or something similar that explains when we must do what and not vague "should" wording.
Ok if you want to avoid "should" then I would rather write:
The caller must make sure that there is a reference to the fence until the callback is called or removed.
Yeah but is that really the case? If you never remove the callback yourself and instead just wait until the cb is called, then that should be safe? Assuming you don't look at the fence afterwards at all. It's just that in practice there's tons of reasons why you might need to bail out and remove the cb, and at that point you can race and need your own reference, or things go boom.
So there's no unconditional requirement to hold a reference.
Yeah and exactly because of this I want to document that you *must* keep a reference around because people tend to get this stuff wrong if you are not strict about it and it works in some cases but not others.
Well I think docs should explain why/when you must hold a reference, like "must hold a reference if", but also explain that the call itself doesn't really require it's own reference that you need to drop in the callback. Hence the distinction of what's strictly needed, and what's needed in most practical cases.
But exactly that is what I want to avoid here.
Not holding a reference you drop when the callback is signaled puts that burden onto the driver instead and that is not really defensive either.
When you install a callback on an object it is good practice to making sure that you have a reference to the object and keep that reference alive until you can be sure that the callback won't be called any more.
Christian.
-Daniel
Christian.
-Daniel
Christian.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @cb will be initialized by dma_fence_add_callback(), no initialization
- by the caller is required. Any number of callbacks can be registered
- to a fence, but a callback can only be registered to one fence at a time.
- Note that the callback can be called from an atomic context. If
- fence is already signaled, this function will return -ENOENT (and
- If fence is already signaled, this function will return -ENOENT (and
- *not* call the callback).
- Add a software callback to the fence. Same restrictions apply to
- refcount as it does to dma_fence_wait(), however the caller doesn't need to
- keep a refcount to fence afterward dma_fence_add_callback() has returned:
- when software access is enabled, the creator of the fence is required to keep
- the fence alive until after it signals with dma_fence_signal(). The callback
- itself can be called from irq context.
- Note that the callback can be called from an atomic context or irq context.
- Returns 0 in case of success, -ENOENT if the fence is already signaled
- and -EINVAL in case of error.
-- 2.25.1
On Wed, Jul 21, 2021 at 11:21:32AM +0200, Christian König wrote:
This callback is pretty much deprecated and should not be used by new implementations.
Clarify that in the documentation as well.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence.h | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 6ffb4b2c6371..a44e42b86c2a 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -214,19 +214,15 @@ struct dma_fence_ops { * Custom wait implementation, defaults to dma_fence_default_wait() if * not set. *
* The dma_fence_default_wait implementation should work for any fence, as long* as @enable_signaling works correctly. This hook allows drivers to* have an optimized version for the case where a process context is* already available, e.g. if @enable_signaling for the general case* needs to set up a worker thread.
* Deprecated and should not be used by new implementations. Only used* by existing implementations which need special handling for their* hardware reset procedure.
I thought the other reason was unreliable interrupts, so waiters had to wake up once per jiffy? So maybe: "... for their hw reset procedure or for handling missed interrupts".
Maybe also add here:
New implementations must have an @enable_signalling and dma_fence_signal() implementation which does not require special handling to make dma_fence_wait() work.
With these:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
* * Must return -ERESTARTSYS if the wait is intr = true and the wait was * interrupted, and remaining jiffies if fence has signaled, or 0 if wait * timed out. Can also return other error values on custom implementations, * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that.
** This callback is optional.-- 2.25.1
dri-devel@lists.freedesktop.org
-
Christian König -
Daniel Vetter