[PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Bart Van Assche]

Instead of assuming that dma_resv_lock() only returns 0 or -EDEADLK,
handle all possible dma_resv_lock() return values. This patch prepares
for enabling compile-time thread-safety analysis. This will cause the
compiler to check whether all dma_resv_lock() return values are handled.

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/dma-buf/dma-resv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index bea3e9858aca..b4710f730e9b 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -792,6 +792,8 @@ static int __init dma_resv_lockdep(void)
 	ret = dma_resv_lock(&obj, &ctx);
 	if (ret == -EDEADLK)
 		dma_resv_lock_slow(&obj, &ctx);
+	else if (ret)
+		goto fini;
 	fs_reclaim_acquire(GFP_KERNEL);
 	/* for unmap_mapping_range on trylocked buffer objects in shrinkers */
 	i_mmap_lock_write(&mapping);
@@ -805,12 +807,14 @@ static int __init dma_resv_lockdep(void)
 #endif
 	fs_reclaim_release(GFP_KERNEL);
 	ww_mutex_unlock(&obj.lock);
+
+fini:
 	ww_acquire_fini(&ctx);
 	mmap_read_unlock(mm);
 
 	mmput(mm);
 
-	return 0;
+	return ret;
 }
 subsys_initcall(dma_resv_lockdep);
 #endif

[PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Bart Van Assche]

Instead of assuming that dma_resv_lock() only returns 0 or -EDEADLK,
handle all possible dma_resv_lock() return values. This patch prepares
for enabling compile-time thread-safety analysis. This will cause the
compiler to check whether all dma_resv_lock() return values are handled.

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/dma-buf/dma-resv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index bea3e9858aca..b4710f730e9b 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -792,6 +792,8 @@ static int __init dma_resv_lockdep(void)
 	ret = dma_resv_lock(&obj, &ctx);
 	if (ret == -EDEADLK)
 		dma_resv_lock_slow(&obj, &ctx);
+	else if (ret)
+		goto fini;
 	fs_reclaim_acquire(GFP_KERNEL);
 	/* for unmap_mapping_range on trylocked buffer objects in shrinkers */
 	i_mmap_lock_write(&mapping);
@@ -805,12 +807,14 @@ static int __init dma_resv_lockdep(void)
 #endif
 	fs_reclaim_release(GFP_KERNEL);
 	ww_mutex_unlock(&obj.lock);
+
+fini:
 	ww_acquire_fini(&ctx);
 	mmap_read_unlock(mm);
 
 	mmput(mm);
 
-	return 0;
+	return ret;
 }
 subsys_initcall(dma_resv_lockdep);
 #endif

[PATCH 05/62] dma-buf: Convert dma_buf_import_sync_file() to the early-return style

[Bart Van Assche]

From: Bart Van Assche <bvanassche@acm.org>

Before making changes in dma_buf_import_sync_file(), convert it to
the early-return coding style. No functionality has been changed.

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/dma-buf/dma-buf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 11711874a325..1666133ac8b8 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -523,11 +523,13 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf,
 		dma_resv_lock(dmabuf->resv, NULL);
 
 		ret = dma_resv_reserve_fences(dmabuf->resv, num_fences);
-		if (!ret) {
-			dma_fence_unwrap_for_each(f, &iter, fence)
-				dma_resv_add_fence(dmabuf->resv, f, usage);
-		}
+		if (ret)
+			goto unlock;
+
+		dma_fence_unwrap_for_each(f, &iter, fence)
+			dma_resv_add_fence(dmabuf->resv, f, usage);
 
+unlock:
 		dma_resv_unlock(dmabuf->resv);
 	}
 

[PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Bart Van Assche]

From: Bart Van Assche <bvanassche@acm.org>

Instead of assuming that dma_resv_lock() only returns 0 or -EDEADLK,
handle all possible dma_resv_lock() return values. This patch prepares
for enabling compile-time thread-safety analysis. This will cause the
compiler to check whether all dma_resv_lock() return values are handled.

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/dma-buf/dma-resv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index bea3e9858aca..b4710f730e9b 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -792,6 +792,8 @@ static int __init dma_resv_lockdep(void)
 	ret = dma_resv_lock(&obj, &ctx);
 	if (ret == -EDEADLK)
 		dma_resv_lock_slow(&obj, &ctx);
+	else if (ret)
+		goto fini;
 	fs_reclaim_acquire(GFP_KERNEL);
 	/* for unmap_mapping_range on trylocked buffer objects in shrinkers */
 	i_mmap_lock_write(&mapping);
@@ -805,12 +807,14 @@ static int __init dma_resv_lockdep(void)
 #endif
 	fs_reclaim_release(GFP_KERNEL);
 	ww_mutex_unlock(&obj.lock);
+
+fini:
 	ww_acquire_fini(&ctx);
 	mmap_read_unlock(mm);
 
 	mmput(mm);
 
-	return 0;
+	return ret;
 }
 subsys_initcall(dma_resv_lockdep);
 #endif

Re: [PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Christian König]

On 2/23/26 22:48, Bart Van Assche wrote:
> Instead of assuming that dma_resv_lock() only returns 0 or -EDEADLK,
> handle all possible dma_resv_lock() return values. This patch prepares
> for enabling compile-time thread-safety analysis. This will cause the
> compiler to check whether all dma_resv_lock() return values are handled.
> 
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/dma-buf/dma-resv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index bea3e9858aca..b4710f730e9b 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -792,6 +792,8 @@ static int __init dma_resv_lockdep(void)
>         ret = dma_resv_lock(&obj, &ctx);
>         if (ret == -EDEADLK)
>                 dma_resv_lock_slow(&obj, &ctx);
> +       else if (ret)
> +               goto fini;

Mhm, that looks a bit questionable to me. Only checking for EDEADLK was intentional.

>         fs_reclaim_acquire(GFP_KERNEL);
>         /* for unmap_mapping_range on trylocked buffer objects in shrinkers */
>         i_mmap_lock_write(&mapping);
> @@ -805,12 +807,14 @@ static int __init dma_resv_lockdep(void)
>  #endif
>         fs_reclaim_release(GFP_KERNEL);
>         ww_mutex_unlock(&obj.lock);
> +
> +fini:
>         ww_acquire_fini(&ctx);
>         mmap_read_unlock(mm);
> 
>         mmput(mm);
> 

> -       return 0;
> +       return ret;

Oh! This is an *absolutely* clear NAK for that change.

This code here is to only improve debugging messages, so it's not problematic at all if it fails.

If I'm not completely mistaken if you return the error code the whole kernel would fail to boot *and* we have a random error injection feature for testing which makes it returning an error randomly!

So this change here would result in randomly not booting kernels and that is something we seriously can't do :)

We could add code which prints and error if ret is something else than 0 or EDEADLK, but that case is already covered by the selftests and so would be 100% dead code.

Why exactly do you need that? Would a code comment explaining the background be helpful?

Regards,
Christian.

>  }
>  subsys_initcall(dma_resv_lockdep);
>  #endif

Re: [PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Bart Van Assche]

On 2/24/26 12:20 AM, Christian König wrote:
> Why exactly do you need that? Would a code comment explaining the
> background be helpful?
dma_resv_lock() has been annotated with	__cond_acquires(0, &obj->lock)
and dma_resv_unlock() has been annotated with __releases(&obj->lock).
Hence, with thread-safety analysis enabled, Clang complains about
dma_resv_unlock() calls if dma_resv_lock() returns an error code. There
are several possibilities for suppressing Clang's thread-
safety warnings:
* The least elegant is to annotate dma_resv_lockdep() with
   __no_context_analysis and to add a comment.
* A better option is to rework the code such that dma_resv_unlock() is
   not called if dma_resv_lock() returns an error code. That is what I
   tried to do. There may be better alternatives than my patch.

Thanks,

Bart.

Re: [PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Christian König]

Hi Bart,

On 2/24/26 18:28, Bart Van Assche wrote:
> On 2/24/26 12:20 AM, Christian König wrote:
>> Why exactly do you need that? Would a code comment explaining the
>> background be helpful?
> dma_resv_lock() has been annotated with    __cond_acquires(0, &obj->lock)
> and dma_resv_unlock() has been annotated with __releases(&obj->lock).
> Hence, with thread-safety analysis enabled, Clang complains about
> dma_resv_unlock() calls if dma_resv_lock() returns an error code. There
> are several possibilities for suppressing Clang's thread-
> safety warnings:
> * The least elegant is to annotate dma_resv_lockdep() with
>   __no_context_analysis and to add a comment.

Yeah, that is clearly a bad idea.

There are basically only two places were we have to ignore this:

1. The dma_resv_lockdep() function you stumbled over.

2. Some selftest we used to have which intentionally got it wrong to check if lockdep correctly complains about it.
   (You most likely already excluded that somehow).

Every other code should definitely have this analysis.

Is it somehow possible to annotate only the dma_resv_lockdep() function?

> * A better option is to rework the code such that dma_resv_unlock() is
>   not called if dma_resv_lock() returns an error code. That is what I
>   tried to do. There may be better alternatives than my patch.

That would be dead code which is never used at all and only there to silence the warning. That is usually also a rather bad idea.

Would it help if we change the code like this?

        ret = dma_resv_lock(&obj, &ctx);
-       if (ret == -EDEADLK)
+       /* Only EDEADLK from the error injection is possible here */
+       if (ret)
                dma_resv_lock_slow(&obj, &ctx);

Thanks,
Christian.

> 
> Thanks,
> 
> Bart.

Re: [PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Bart Van Assche]

On 2/25/26 12:16 AM, Christian König wrote:
> Would it help if we change the code like this?
> 
>          ret = dma_resv_lock(&obj, &ctx);
> -       if (ret == -EDEADLK)
> +       /* Only EDEADLK from the error injection is possible here */
> +       if (ret)
>                  dma_resv_lock_slow(&obj, &ctx);

Yes, the above is sufficient to suppress the Clang thread-safety warning
for dma_resv_lockdep(). Is a source code comment about EDEADLK preferred
or would the following perhaps also be acceptable?

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index bea3e9858aca..4d65dddbcbdf 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -790,8 +790,10 @@ static int __init dma_resv_lockdep(void)
         mmap_read_lock(mm);
         ww_acquire_init(&ctx, &reservation_ww_class);
         ret = dma_resv_lock(&obj, &ctx);
-       if (ret == -EDEADLK)
+       if (ret) {
+               WARN_ON_ONCE(ret != -EDEADLK);
                 dma_resv_lock_slow(&obj, &ctx);
+       }
         fs_reclaim_acquire(GFP_KERNEL);
         /* for unmap_mapping_range on trylocked buffer objects in 
shrinkers */
         i_mmap_lock_write(&mapping);

Thanks,

Bart.

Re: [PATCH 06/62] dma-buf: Handle all dma_resv_lock() errors

[Christian König]

On 2/25/26 22:36, Bart Van Assche wrote:
> On 2/25/26 12:16 AM, Christian König wrote:
>> Would it help if we change the code like this?
>>
>>          ret = dma_resv_lock(&obj, &ctx);
>> -       if (ret == -EDEADLK)
>> +       /* Only EDEADLK from the error injection is possible here */
>> +       if (ret)
>>                  dma_resv_lock_slow(&obj, &ctx);
> 
> Yes, the above is sufficient to suppress the Clang thread-safety warning
> for dma_resv_lockdep().

Cool, then let us use this approach here.

> Is a source code comment about EDEADLK preferred
> or would the following perhaps also be acceptable?

I think the warning is optional, but potentially good to have as well.

But the code comment is mandatory, otherwise we will have forgotten in 10 years why exactly only EDEADLK can happen here.

> 
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index bea3e9858aca..4d65dddbcbdf 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -790,8 +790,10 @@ static int __init dma_resv_lockdep(void)
>         mmap_read_lock(mm);
>         ww_acquire_init(&ctx, &reservation_ww_class);
>         ret = dma_resv_lock(&obj, &ctx);
> -       if (ret == -EDEADLK)
> +       if (ret) {
> +               WARN_ON_ONCE(ret != -EDEADLK);

Please make that only WARN_ON(). I would be massively surprised when a subsystem initcall executes more than once.

Thanks,
Christian.

>                 dma_resv_lock_slow(&obj, &ctx);
> +       }
>         fs_reclaim_acquire(GFP_KERNEL);
>         /* for unmap_mapping_range on trylocked buffer objects in shrinkers */
>         i_mmap_lock_write(&mapping);
> 
> Thanks,
> 
> Bart.