drm/nouveau: Don't set signaled fences' error codes
Subsystems (7)
v2 cover drm/nouveau: Don't set signaled fences' error codes
Changes in v2:
- Only fix the issue by checking for a fence being signaled in nouveau_fence_context_kill(), before setting the fence's error. (Christian, Danilo)
- Drop cleanup patches. Instead, idiomaticize for-each-loops.
Was called "Fix & improve nouveau_fence_done()" before.
I've tested this with KASAN & kmemleak.
P.
Philipp Stanner (2):
drm/nouveau: Fix WARN_ON in nouveau_fence_context_kill() drm/nouveau: nouveau_fence: Standardize list iterations
drivers/gpu/drm/nouveau/nouveau_fence.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Signature · 1 line
2.48.1
PATCH v2 1/2 drm/nouveau: Fix WARN_ON in nouveau_fence_context_kill()
Nouveau is mostly designed in a way that it's expected that fences only ever get signaled through nouveau_fence_signal(). However, in at least one other place, nouveau_fence_done(), can signal fences, too. If that happens (race) a signaled fence remains in the pending list for a while, until it gets removed by nouveau_fence_update().
Should nouveau_fence_context_kill() run in the meantime, this would be a bug because the function would attempt to set an error code on an already signaled fence.
Have nouveau_fence_context_kill() check for a fence being signaled.
--- drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/nouveau/nouveau_fence.c +1 -1
@@ -90,7 +90,7 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error)while (!list_empty (&fctx->pending)) { fence =list_entry (fctx->pending.next,typeof (*fence), head); -if (error) +if (error && !dma_fence_is_signaled_locked (&fence->base))dma_fence_set_error (&fence->base, error);if (nouveau_fence_signal (fence))
Signature · 1 line
2.48.1
Re: [PATCH v2 1/2] drm/nouveau: Fix WARN_ON in nouveau_fence_context_kill()
Quoted 10 lines · Philipp Stanner
Nouveau is mostly designed in a way that it's expected that fences only ever get signaled through
nouveau_fence_signal(). However, in at least one other place,nouveau_fence_done(), can signal fences, too. If that happens (race) a signaled fence remains in the pending list for a while, until it gets removed bynouveau_fence_update().Should
nouveau_fence_context_kill()run in the meantime, this would be a bug because the function would attempt to set an error code on an already signaled fence.
Quoted 6 lines
Have
nouveau_fence_context_kill()check for a fence being signaled.Cc <[email protected]> # v5.10+Fixes ea13e5abf807 ("drm/nouveau: signal pending fences when channel has been killed")Suggested-by Christian König <[email protected]>Signed-off-by Philipp Stanner <[email protected]>
Applied to drm-misc-fixes, thanks!
PATCH v2 2/2 drm/nouveau: nouveau_fence: Standardize list iterations
In reply to Philipp Stannernouveau_fence.c iterates over lists in a non-canonical way. Since the operations done are just basic for-each-loops, they should be written in the standard form.
Use for_each_safe() instead of the custom loop iterations.
--- drivers/gpu/drm/nouveau/nouveau_fence.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/gpu/drm/nouveau/nouveau_fence.c +6 -4
@@ -84,11 +84,12 @@ void nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) {struct nouveau_fence *fence; +struct list_head *pos, *tmp;unsigned long flags;spin_lock_irqsave (&fctx->lock, flags); -while (!list_empty (&fctx->pending)) { - fence =list_entry (fctx->pending.next,typeof (*fence), head); +list_for_each_safe (pos, tmp, &fctx->pending) { + fence = list_entry(pos, struct nouveau_fence, head);if (error && !dma_fence_is_signaled_locked (&fence->base))dma_fence_set_error (&fence->base, error);
@@ -131,11 +132,12 @@ static int nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx) { struct nouveau_fence *fence; +struct list_head *pos, *tmp;int drop =0 ;u32 seq = fctx->read (chan); -while (!list_empty (&fctx->pending)) { - fence =list_entry (fctx->pending.next,typeof (*fence), head); +list_for_each_safe (pos, tmp, &fctx->pending) { + fence = list_entry(pos, struct nouveau_fence, head);if ((int )(seq - fence->base.seqno) <0 )break ;
Signature · 1 line
2.48.1
Re: [PATCH v2 2/2] drm/nouveau: nouveau_fence: Standardize list iterations
Am 15.04.25 um 14:19 schrieb Philipp Stanner:
Quoted 7 lines
nouveau_fence.citerates over lists in a non-canonical way. Since the operations done are just basic for-each-loops, they should be written in the standard form.Use
for_each_safe()instead of the custom loop iterations.Signed-off-by Philipp Stanner <[email protected]>
Quoted 38 lines
--- drivers/gpu/drm/nouveau/nouveau_fence.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)drivers/gpu/drm/nouveau/nouveau_fence.c +6
-4@@ -84,11 +84,12 @@ void nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) {struct nouveau_fence *fence; +struct list_head *pos, *tmp;unsigned long flags;spin_lock_irqsave (&fctx->lock, flags); -while (!list_empty (&fctx->pending)) { - fence =list_entry (fctx->pending.next,typeof (*fence), head); +list_for_each_safe (pos, tmp, &fctx->pending) { + fence = list_entry(pos, struct nouveau_fence, head);if (error && !dma_fence_is_signaled_locked (&fence->base))dma_fence_set_error (&fence->base, error);@@ -131,11 +132,12 @@ static int nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx) { struct nouveau_fence *fence; +struct list_head *pos, *tmp;int drop =0 ;u32 seq = fctx->read (chan); -while (!list_empty (&fctx->pending)) { - fence =list_entry (fctx->pending.next,typeof (*fence), head); +list_for_each_safe (pos, tmp, &fctx->pending) { + fence = list_entry(pos, struct nouveau_fence, head);if ((int )(seq - fence->base.seqno) <0 )break ;
Danilo Krummrich
maintainerRe: [PATCH v2 2/2] drm/nouveau: nouveau_fence: Standardize list iterations
In reply to Philipp StannerQuoted 5 lines · Philipp Stanner
nouveau_fence.citerates over lists in a non-canonical way. Since the operations done are just basic for-each-loops, they should be written in the standard form.Use
for_each_safe()instead of the custom loop iterations.
Please use list_for_each_entry_safe() instead.
You only need resend this patch, I will pick patch 1 for -fixes; this one goes into -next.
- Danilo