Harden the dma-fence documentation a bit more

Harden the dma-fence documentation a bit more

[Christian König]

Hi guys,

while it is in most cases technically possible to not have a reference to the dma_fence when adding a callback it is usually a good idea to make sure to always have a reference anyway.

Otherwise we can indeed see cases where this doesn't really work as intended like for example in the now fixed EPOLL code.

Regards,
Christian.

[PATCH 1/2] dma-buf: clarify dma_fence_ops->wait documentation

[Christian König]

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);
-- 
2.25.1

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

[Christian König]

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

Re: [PATCH 1/2] dma-buf: clarify dma_fence_ops->wait documentation

[Daniel Vetter]

On Wed, Sep 01, 2021 at 02:02:39PM +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>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  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);
> -- 
> 2.25.1
> 

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

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

[Daniel Vetter]

On Wed, Sep 01, 2021 at 02:02:40PM +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>

Still on the fence between documenting the precise rules and documenting
the safe rules, but this is tricky enough that you got me convinced. Plus
shorter, simpler, clearer kerneldoc has much better chances of being read,
understood and followed.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

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

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

[Christian König]

Am 02.09.21 um 16:42 schrieb Daniel Vetter:
> On Wed, Sep 01, 2021 at 02:02:40PM +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>
> Still on the fence between documenting the precise rules and documenting
> the safe rules, but this is tricky enough that you got me convinced. Plus
> shorter, simpler, clearer kerneldoc has much better chances of being read,
> understood and followed.

I think that for documentation we should apply the same rules we have 
for code.

E.g. keep it simple until you absolutely have to make it complex and 
keep it defensive with the least probability for something to go wrong.

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks,
Christian.

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

Re: [PATCH 1/2] dma-buf: clarify dma_fence_ops->wait documentation

[Daniel Vetter]

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.
>  	 */
>  	signed long (*wait)(struct dma_fence *fence,
>  			    bool intr, signed long timeout);
> -- 
> 2.25.1
> 

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

[PATCH 1/2] dma-buf: clarify dma_fence_ops->wait documentation

[Christian König]

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);
-- 
2.25.1