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

[Christian König]

Am 21.07.21 um 16:37 schrieb Daniel Vetter:
> On Wed, Jul 21, 2021 at 3:57 PM Christian König
> <ckoenig.leichtzumerken at 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 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.
> 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 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
>>>>>>
>