[PATCH 2/2] dma-buf: clarify dma_fence_add_callback documentation

[Daniel Vetter]

On Wed, Jul 21, 2021 at 3:18 PM Christian König
<ckoenig.leichtzumerken at 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 at 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 at 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
> >>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch