[PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Tvrtko Ursulin]

Drm_sched_job_add_dependency() consumes the fence reference both on
success and failure, so in the latter case the dma_fence_put() on the
error path (xarray failed to expand) is a double free.

Interestingly this bug appears to have been present ever since
ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
then looked like this:

drm_sched_job_add_implicit_dependencies():
...
       for (i = 0; i < fence_count; i++) {
               ret = drm_sched_job_add_dependency(job, fences[i]);
               if (ret)
                       break;
       }

       for (; i < fence_count; i++)
               dma_fence_put(fences[i]);

Which means for the failing 'i' the dma_fence_put was already a double
free. Possibly there were no users at that time, or the test cases were
insufficient to hit it.

The bug was then only noticed and fixed after
9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
landed, with its fixup of
4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").

At that point it was a slightly different flavour of a double free, which
963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
noticed and attempted to fix.

But it only moved the double free from happening inside the
drm_sched_job_add_dependency(), when releasing the reference not yet
obtained, to the caller, when releasing the reference already released by
the former in the failure case.

As such it is not easy to identify the right target for the fixes tag so
lets keep it simple and just continue the chain.

We also drop the misleading comment about additional reference, since it
is not additional but the only one from the point of view of dependency
tracking.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.16+
---
 drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 46119aacb809..aff34240f230 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
 {
 	struct dma_resv_iter cursor;
 	struct dma_fence *fence;
-	int ret;
+	int ret = 0;
 
 	dma_resv_assert_held(resv);
 
 	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
-		/* Make sure to grab an additional ref on the added fence */
-		dma_fence_get(fence);
-		ret = drm_sched_job_add_dependency(job, fence);
-		if (ret) {
-			dma_fence_put(fence);
-			return ret;
-		}
+		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
+		if (ret)
+			break;
 	}
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(drm_sched_job_add_resv_dependencies);
 
-- 
2.48.0

Re: [PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Tvrtko Ursulin]

A gentle ping - any takers to double check my analysis and review the below?

Regards,

Tvrtko

On 03/10/2025 10:26, Tvrtko Ursulin wrote:
> Drm_sched_job_add_dependency() consumes the fence reference both on
> success and failure, so in the latter case the dma_fence_put() on the
> error path (xarray failed to expand) is a double free.
> 
> Interestingly this bug appears to have been present ever since
> ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
> then looked like this:
> 
> drm_sched_job_add_implicit_dependencies():
> ...
>         for (i = 0; i < fence_count; i++) {
>                 ret = drm_sched_job_add_dependency(job, fences[i]);
>                 if (ret)
>                         break;
>         }
> 
>         for (; i < fence_count; i++)
>                 dma_fence_put(fences[i]);
> 
> Which means for the failing 'i' the dma_fence_put was already a double
> free. Possibly there were no users at that time, or the test cases were
> insufficient to hit it.
> 
> The bug was then only noticed and fixed after
> 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
> landed, with its fixup of
> 4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").
> 
> At that point it was a slightly different flavour of a double free, which
> 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> noticed and attempted to fix.
> 
> But it only moved the double free from happening inside the
> drm_sched_job_add_dependency(), when releasing the reference not yet
> obtained, to the caller, when releasing the reference already released by
> the former in the failure case.
> 
> As such it is not easy to identify the right target for the fixes tag so
> lets keep it simple and just continue the chain.
> 
> We also drop the misleading comment about additional reference, since it
> is not additional but the only one from the point of view of dependency
> tracking.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.16+
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 46119aacb809..aff34240f230 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>   {
>   	struct dma_resv_iter cursor;
>   	struct dma_fence *fence;
> -	int ret;
> +	int ret = 0;
>   
>   	dma_resv_assert_held(resv);
>   
>   	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
> -		/* Make sure to grab an additional ref on the added fence */
> -		dma_fence_get(fence);
> -		ret = drm_sched_job_add_dependency(job, fence);
> -		if (ret) {
> -			dma_fence_put(fence);
> -			return ret;
> -		}
> +		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
> +		if (ret)
> +			break;
>   	}
> -	return 0;
> +	return ret;
>   }
>   EXPORT_SYMBOL(drm_sched_job_add_resv_dependencies);
>   

Re: [PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Philipp Stanner]

On Fri, 2025-10-03 at 10:26 +0100, Tvrtko Ursulin wrote:
> Drm_sched_job_add_dependency() consumes the fence reference both on

s/D/d

> success and failure, so in the latter case the dma_fence_put() on the
> error path (xarray failed to expand) is a double free.
> 
> Interestingly this bug appears to have been present ever since
> ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
> then looked like this:
> 
> drm_sched_job_add_implicit_dependencies():
> ...
>        for (i = 0; i < fence_count; i++) {
>                ret = drm_sched_job_add_dependency(job, fences[i]);
>                if (ret)
>                        break;
>        }
> 
>        for (; i < fence_count; i++)
>                dma_fence_put(fences[i]);
> 
> Which means for the failing 'i' the dma_fence_put was already a double
> free. Possibly there were no users at that time, or the test cases were
> insufficient to hit it.
> 
> The bug was then only noticed and fixed after
> 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
> landed, with its fixup of
> 4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").
> 
> At that point it was a slightly different flavour of a double free, which
> 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> noticed and attempted to fix.
> 
> But it only moved the double free from happening inside the
> drm_sched_job_add_dependency(), when releasing the reference not yet
> obtained, to the caller, when releasing the reference already released by
> the former in the failure case.

That's certainly interesting, but is there a specific reason why you
include all of that?

The code is as is, and AFAICS it's just a bug stemming from original
bugs present and then refactorings happening.

I would at least remove the old 'implicit_dependencies' function from
the commit message. It's just confusing and makes one look for that in
the current code or patch.

> 
> As such it is not easy to identify the right target for the fixes tag so
> lets keep it simple and just continue the chain.
> 
> We also drop the misleading comment about additional reference, since it
> is not additional but the only one from the point of view of dependency
> tracking.


IMO that comment is nonsense. It's useless, too, because I can *see*
that a reference is being taken there, but not *why*.

Argh, these comments. See also my commit 72ebc18b34993


Anyways. Removing it is fine, but adding a better comment is better.
See below.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

Is there an error report that could be included here with a Closes:
tag?

> Cc: Christian König <christian.koenig@amd.com>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.16+
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 46119aacb809..aff34240f230 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>  {
>  	struct dma_resv_iter cursor;
>  	struct dma_fence *fence;
> -	int ret;
> +	int ret = 0;
>  
>  	dma_resv_assert_held(resv);
>  
>  	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
> -		/* Make sure to grab an additional ref on the added fence */
> -		dma_fence_get(fence);
> -		ret = drm_sched_job_add_dependency(job, fence);
> -		if (ret) {
> -			dma_fence_put(fence);
> -			return ret;
> -		}
> +		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));

You still take a reference as before, but there is no comment anymore.
Can you add one explaining why a new reference is taken here?

I guess it will be something like "This needs a new reference for the
job", since you cannot rely on the one from resv.

> +		if (ret)
> +			break;
>  	}
> -	return 0;
> +	return ret;


That's an unnecessarily enlargement of the git diff because of style,
isn't it? Better keep the diff minimal here for git blame.


P.


>  }
>  EXPORT_SYMBOL(drm_sched_job_add_resv_dependencies);
>  

Re: [PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Tvrtko Ursulin]

On 13/10/2025 15:39, Philipp Stanner wrote:
> On Fri, 2025-10-03 at 10:26 +0100, Tvrtko Ursulin wrote:
>> Drm_sched_job_add_dependency() consumes the fence reference both on
> 
> s/D/d

I cannot find the source right now but I am 99% sure someone pulled it 
on me in the past and educated me sentences should always start with a 
capital letter, even when it is not a proper word. Because in the past I 
was always doing the same as you suggest. I don't mind TBH.
>> success and failure, so in the latter case the dma_fence_put() on the
>> error path (xarray failed to expand) is a double free.
>>
>> Interestingly this bug appears to have been present ever since
>> ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
>> then looked like this:
>>
>> drm_sched_job_add_implicit_dependencies():
>> ...
>>         for (i = 0; i < fence_count; i++) {
>>                 ret = drm_sched_job_add_dependency(job, fences[i]);
>>                 if (ret)
>>                         break;
>>         }
>>
>>         for (; i < fence_count; i++)
>>                 dma_fence_put(fences[i]);
>>
>> Which means for the failing 'i' the dma_fence_put was already a double
>> free. Possibly there were no users at that time, or the test cases were
>> insufficient to hit it.
>>
>> The bug was then only noticed and fixed after
>> 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
>> landed, with its fixup of
>> 4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").
>>
>> At that point it was a slightly different flavour of a double free, which
>> 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
>> noticed and attempted to fix.
>>
>> But it only moved the double free from happening inside the
>> drm_sched_job_add_dependency(), when releasing the reference not yet
>> obtained, to the caller, when releasing the reference already released by
>> the former in the failure case.
> 
> That's certainly interesting, but is there a specific reason why you
> include all of that?

Yes, because bug was attempted to be fixed two times already. So it is 
IMO worth having a write up on the history.
> The code is as is, and AFAICS it's just a bug stemming from original
> bugs present and then refactorings happening.
> 
> I would at least remove the old 'implicit_dependencies' function from
> the commit message. It's just confusing and makes one look for that in
> the current code or patch.It does say "back then" and it references the commit so shouldn't be 
confusing.

The discussion is long for an additional reason that Fixes: targets not 
the original commit which did not get this right, but the last change 
which tried to fix it but did not. It sounded more logical to continue 
the chain on fixes but dunno. Could replace it with the original one 
just as well.
>> As such it is not easy to identify the right target for the fixes tag so
>> lets keep it simple and just continue the chain.
>>
>> We also drop the misleading comment about additional reference, since it
>> is not additional but the only one from the point of view of dependency
>> tracking.
> 
> 
> IMO that comment is nonsense. It's useless, too, because I can *see*
> that a reference is being taken there, but not *why*.
> 
> Argh, these comments. See also my commit 72ebc18b34993
> 
> 
> Anyways. Removing it is fine, but adding a better comment is better.
> See below.
> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Is there an error report that could be included here with a Closes:
> tag?No, it did not come from any such system. Could perhaps put a link to 
the report as Reference: 
https://lore.kernel.org/dri-devel/aNFbXq8OeYl3QSdm@stanley.mountain/

?

>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Rob Clark <robdclark@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.16+
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 46119aacb809..aff34240f230 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>>   {
>>   	struct dma_resv_iter cursor;
>>   	struct dma_fence *fence;
>> -	int ret;
>> +	int ret = 0;
>>   
>>   	dma_resv_assert_held(resv);
>>   
>>   	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
>> -		/* Make sure to grab an additional ref on the added fence */
>> -		dma_fence_get(fence);
>> -		ret = drm_sched_job_add_dependency(job, fence);
>> -		if (ret) {
>> -			dma_fence_put(fence);
>> -			return ret;
>> -		}
>> +		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
> 
> You still take a reference as before, but there is no comment anymore.
> Can you add one explaining why a new reference is taken here?
> 
> I guess it will be something like "This needs a new reference for the
> job", since you cannot rely on the one from resv.

I was mulling it over but then thought the kerneldoc for 
drm_sched_job_add_dependency() is good enough since it explains the 
function always consumes the reference and dma_resv_for_each_fence() 
does not hold one over the iteration. I can add it.

> 
>> +		if (ret)
>> +			break;
>>   	}
>> -	return 0;
>> +	return ret;
> 
> 
> That's an unnecessarily enlargement of the git diff because of style,
> isn't it? Better keep the diff minimal here for git blame.

Given the relative size of the diff versus the function itself it did 
not look like a big deal to reduce to one return statement while 
touching it, but I can keep two returns just as well, no problem.

Regards,

Tvrtko

Re: [PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Matthew Brost]

On Mon, Oct 13, 2025 at 06:01:49PM +0100, Tvrtko Ursulin wrote:
> 
> On 13/10/2025 15:39, Philipp Stanner wrote:
> > On Fri, 2025-10-03 at 10:26 +0100, Tvrtko Ursulin wrote:
> > > Drm_sched_job_add_dependency() consumes the fence reference both on
> > 
> > s/D/d
> 
> I cannot find the source right now but I am 99% sure someone pulled it on me
> in the past and educated me sentences should always start with a capital
> letter, even when it is not a proper word. Because in the past I was always
> doing the same as you suggest. I don't mind TBH.

Drm_sched_job_add_dependency breaks grep / searches, so I'm with Philipp s/D/d

Matt

> > > success and failure, so in the latter case the dma_fence_put() on the
> > > error path (xarray failed to expand) is a double free.
> > > 
> > > Interestingly this bug appears to have been present ever since
> > > ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
> > > then looked like this:
> > > 
> > > drm_sched_job_add_implicit_dependencies():
> > > ...
> > >         for (i = 0; i < fence_count; i++) {
> > >                 ret = drm_sched_job_add_dependency(job, fences[i]);
> > >                 if (ret)
> > >                         break;
> > >         }
> > > 
> > >         for (; i < fence_count; i++)
> > >                 dma_fence_put(fences[i]);
> > > 
> > > Which means for the failing 'i' the dma_fence_put was already a double
> > > free. Possibly there were no users at that time, or the test cases were
> > > insufficient to hit it.
> > > 
> > > The bug was then only noticed and fixed after
> > > 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
> > > landed, with its fixup of
> > > 4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").
> > > 
> > > At that point it was a slightly different flavour of a double free, which
> > > 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> > > noticed and attempted to fix.
> > > 
> > > But it only moved the double free from happening inside the
> > > drm_sched_job_add_dependency(), when releasing the reference not yet
> > > obtained, to the caller, when releasing the reference already released by
> > > the former in the failure case.
> > 
> > That's certainly interesting, but is there a specific reason why you
> > include all of that?
> 
> Yes, because bug was attempted to be fixed two times already. So it is IMO
> worth having a write up on the history.
> > The code is as is, and AFAICS it's just a bug stemming from original
> > bugs present and then refactorings happening.
> > 
> > I would at least remove the old 'implicit_dependencies' function from
> > the commit message. It's just confusing and makes one look for that in
> > the current code or patch.It does say "back then" and it references the
> > commit so shouldn't be
> confusing.
> 
> The discussion is long for an additional reason that Fixes: targets not the
> original commit which did not get this right, but the last change which
> tried to fix it but did not. It sounded more logical to continue the chain
> on fixes but dunno. Could replace it with the original one just as well.
> > > As such it is not easy to identify the right target for the fixes tag so
> > > lets keep it simple and just continue the chain.
> > > 
> > > We also drop the misleading comment about additional reference, since it
> > > is not additional but the only one from the point of view of dependency
> > > tracking.
> > 
> > 
> > IMO that comment is nonsense. It's useless, too, because I can *see*
> > that a reference is being taken there, but not *why*.
> > 
> > Argh, these comments. See also my commit 72ebc18b34993
> > 
> > 
> > Anyways. Removing it is fine, but adding a better comment is better.
> > See below.
> > 
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > 
> > Is there an error report that could be included here with a Closes:
> > tag?No, it did not come from any such system. Could perhaps put a link
> > to
> the report as Reference:
> https://lore.kernel.org/dri-devel/aNFbXq8OeYl3QSdm@stanley.mountain/
> 
> ?
> 
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Rob Clark <robdclark@chromium.org>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Philipp Stanner <phasta@kernel.org>
> > > Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v5.16+
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
> > >   1 file changed, 5 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > > index 46119aacb809..aff34240f230 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
> > >   {
> > >   	struct dma_resv_iter cursor;
> > >   	struct dma_fence *fence;
> > > -	int ret;
> > > +	int ret = 0;
> > >   	dma_resv_assert_held(resv);
> > >   	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
> > > -		/* Make sure to grab an additional ref on the added fence */
> > > -		dma_fence_get(fence);
> > > -		ret = drm_sched_job_add_dependency(job, fence);
> > > -		if (ret) {
> > > -			dma_fence_put(fence);
> > > -			return ret;
> > > -		}
> > > +		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
> > 
> > You still take a reference as before, but there is no comment anymore.
> > Can you add one explaining why a new reference is taken here?
> > 
> > I guess it will be something like "This needs a new reference for the
> > job", since you cannot rely on the one from resv.
> 
> I was mulling it over but then thought the kerneldoc for
> drm_sched_job_add_dependency() is good enough since it explains the function
> always consumes the reference and dma_resv_for_each_fence() does not hold
> one over the iteration. I can add it.
> 
> > 
> > > +		if (ret)
> > > +			break;
> > >   	}
> > > -	return 0;
> > > +	return ret;
> > 
> > 
> > That's an unnecessarily enlargement of the git diff because of style,
> > isn't it? Better keep the diff minimal here for git blame.
> 
> Given the relative size of the diff versus the function itself it did not
> look like a big deal to reduce to one return statement while touching it,
> but I can keep two returns just as well, no problem.
> 
> Regards,
> 
> Tvrtko
>