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

[Christian König]

Am 21.07.21 um 15:36 schrieb 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.

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 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
>>>>
>