[PATCH v4 0/3] drm/nouveau: channel-kill event ordering fixes, and lower the gate to NV50

[PATCH v4 0/3] drm/nouveau: channel-kill event ordering fixes, and lower the gate to NV50

[Marek Czernohous]

This is v4. v3 is here:

  https://lore.kernel.org/all/20260812231330.705425-1-mczernohous@gmail.com/

Thank you for the review. Changes since v3:

1/3 unchanged, and now carries Lyude's Reviewed-by.

2/3 rewritten. v3 moved the subscription behind context_new(), which
    traded a half-built fence context for a missed kill. Lyude pointed out
    that the subscription does not have to move at all: give the fence
    context a flag and have the two sides hand the kill over.

3/3 is v3's 4/4, unchanged in code. Lyude asked what testing this has had
    on Tesla and said it will need input from others; the testing question
    is answered under Testing below. It is last in the series so it can be
    dropped without disturbing the two fixes, and I am happy for it to
    wait for that input rather than go with them.

Withdrawn since v3

  v3 carried a patch that demoted one specific CACHE_ERROR to debug
  level and attributed it to a Mesa bind probe. I am withdrawing it,
  because that attribution does not survive a look at the tree. Method
  0x0060 on this class is SET_CONTEXT_DMA_SEMAPHORE
  (nvhw/class/cl826f.h), and the writer is the kernel itself,
  nv84_fence_emit32() and nv84_fence_sync32(), on subchannel 0
  (nvif/push006c.h). The data is the VRAM ctxdma handle that Mesa picks
  (nouveau_screen.c, .vram = 0xbeef0201) and the kernel binds at channel
  creation. All 99 logged occurrences behind that patch read "subc 0
  mthd 0060 data beef0201". Mesa's NV50 Gallium never writes on
  subchannel 0. So this is the driver's own semaphore-context rebind
  being rejected by the puller now and then, and I have not established
  why. I will look at it separately rather than carry it here.

On 2/3, and the two places where it differs from the sketch

  The sketch checks chan->killed before setting ->ready, but each side
  has to store its own flag before loading the other's, or one
  interleaving loses the kill even under sequential consistency;
  swapped, with smp_mb() on both sides, it is the store-buffering
  pattern. And the kill side cannot take fctx->lock, because that lock
  is exactly what must not be touched before the context is built, so
  ->ready is a plain bool read outside the lock, published with release
  and read with acquire. The commit message has the full argument.

On the aside about writing the respin without Claude

  This work is AI assisted, as the Assisted-by trailers say.

  The advice is right, and I would rather say so than let it pass. The
  honest position, though, is that I doubt I would have got to these
  bugs at all without the assistance.

Testing

  Reference hardware: Apple Mac mini Late 2009, MCP79 / GeForce 9400M
  (NVAC), Core 2 Duo, Wayland (labwc).

  Build. Built against the base commit named at the end of this mail.
  The nouveau module compiles and links with W=1 and no new warnings.
  checkpatch.pl --strict is clean on all three patches.

  Deliberate channel kills on Tesla, which is Lyude's question on 3/3.

  The kill path on this hardware has been exercised on purpose, not only
  observed, and under a real 3D workload. A local debug patch, which is
  not part of this series, synthesises a CACHE_ERROR on a nominated
  channel. The victim was SuperTuxKart, with its channel live under
  load. Verbatim, timestamps and unrelated lines elided:

    nouveau 0000:02:00.0: fifo: inject:   ch   2 [labwc[5670]] errored 0
    nouveau 0000:02:00.0: fifo: inject:   ch   3 [labwc[5670]] errored 0
    nouveau 0000:02:00.0: fifo: inject:   ch   4 [Xwayland[242990]] errored 0
    nouveau 0000:02:00.0: fifo: inject:   ch   5 [supertuxkart[1181685]] errored 0
    [...]
    nouveau 0000:02:00.0: fifo: ch 5 fault 1/3 in 10000ms window, skipping method and resuming (Tier-0)
    nouveau 0000:02:00.0: fifo: ch 5 fault 2/3 in 10000ms window, skipping method and resuming (Tier-0)
    nouveau 0000:02:00.0: fifo:000000:0005:0005:[supertuxkart[1181685]] errored - disabling channel
    nouveau 0000:02:00.0: Xwayland[242990]: channel 5 killed!
    supertuxkart[1181685]: segfault at 560800000000 ip 00007f617c0d5ba6 [...] in libgallium

  So on NVAC, with 3/3 in place, the ERRORED event is delivered, the
  handler runs, the fence context is killed, and the compositor is
  unaffected.

  Two limits on what that proves. The escalation that disabled the
  channel is local and not in this series; what this series contributes
  is that the event reaches a subscriber at all instead of being dropped
  into an empty notifier list. And the victim did not survive its own
  channel being killed: it segfaulted inside Mesa rather than handling
  the -ENODEV fences. Without the subscription it would have hung
  instead.

  Games and sustained 3D. glxgears without vsync for ten minutes, and
  SuperTuxKart for a full race, ten and a half minutes, both with 3/3 in
  place. In both runs everything the kernel said came at window creation
  and was the known benign gr DATA_ERROR trap: 71 of those for glxgears,
  three for SuperTuxKart, and none at all once either was running. No
  wedge, no unexpected kill, and the session came through both
  unchanged.

  Soak. Code equivalent to 1/3 and 3/3 has been running on this machine
  since 2026-07-25, across the kernel bumps 7.1.5 through 7.1.8, with no
  regression.

  The 2/3 in this posting has NOT been soaked. What has been running
  since 2026-08-06 is the v3 form of it, which moved the subscription;
  the handover in this version is new as of today. I will report back
  once it has run.

  What the soak does not show, stated plainly: that tree carries local
  patches this series does not, including a cap on the plane-fence wait
  in the nonblocking commit tail. So the soak says these changes do not
  misbehave in daily use. It is not an independent demonstration of the
  failure modes above.

  1/3 and 2/3 are ordering fixes for windows I have not managed to hit
  deliberately; the reasoning is from the source.

Marek Czernohous (3):
  drm/nouveau: unsubscribe the channel-kill event before the fence
    context
  drm/nouveau: don't kill a fence context that is not ready yet
  drm/nouveau: subscribe to channel-kill events on NV50 and newer

 drivers/gpu/drm/nouveau/nouveau_chan.c  | 30 ++++++++++++++++++++-----
 drivers/gpu/drm/nouveau/nouveau_fence.c | 19 ++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_fence.h |  8 +++++++
 3 files changed, 52 insertions(+), 5 deletions(-)


base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.54.0

[PATCH v4 1/3] drm/nouveau: unsubscribe the channel-kill event before the fence context

[Marek Czernohous]

nouveau_channel_del() tears the fence context down first and only drops
the channel-kill subscription later, in the middle of the nvif object
teardown:

	if (chan->fence)
		nouveau_fence(chan->cli->drm)->context_del(chan);
	...
	nvif_object_dtor(&chan->vram);
	nvif_event_dtor(&chan->kill);

The subscribed handler is nouveau_channel_killed(), which calls
nouveau_channel_kill() and from there nouveau_fence_context_kill() on
chan->fence. A kill event delivered in that window takes fctx->lock and
walks fctx->pending on a fence context that context_del() has already
freed.

Nothing reaches this below Fermi today, because the subscription is
gated on FERMI_CHANNEL_GPFIFO and nothing kills a channel there. On
Fermi and newer the window is real but narrow, since a kill has to land
exactly while the channel is being destroyed. That is reason enough on
its own, which is why this carries a Fixes: tag. The last patch in this
series subscribes Tesla channels as well; nothing kills those today, so
it does not widen the exposure now, but it is the groundwork for a
recovery path that would, and the ordering is better fixed before that
lands than alongside it.

Drop the subscription before anything it depends on is torn down.

Fixes: ea13e5abf807 ("drm/nouveau: signal pending fences when channel has been killed")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_chan.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
index 598513f60449..f142f6310596 100644
--- a/drivers/gpu/drm/nouveau/nouveau_chan.c
+++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
@@ -90,6 +90,14 @@ nouveau_channel_del(struct nouveau_channel **pchan)
 {
 	struct nouveau_channel *chan = *pchan;
 	if (chan) {
+		/*
+		 * Drop the kill-event subscription first.  Its handler
+		 * dereferences chan->fence, which the fence context teardown
+		 * below frees, so leaving it armed across the teardown leaves
+		 * a window for a use-after-free.
+		 */
+		nvif_event_dtor(&chan->kill);
+
 		if (chan->fence)
 			nouveau_fence(chan->cli->drm)->context_del(chan);
 
@@ -100,7 +108,6 @@ nouveau_channel_del(struct nouveau_channel **pchan)
 		nvif_object_dtor(&chan->nvsw);
 		nvif_object_dtor(&chan->gart);
 		nvif_object_dtor(&chan->vram);
-		nvif_event_dtor(&chan->kill);
 		nvif_object_dtor(&chan->user);
 		nvif_mem_dtor(&chan->mem_userd);
 		nouveau_vma_del(&chan->sema.vma);
-- 
2.54.0

[PATCH v4 2/3] drm/nouveau: don't kill a fence context that is not ready yet

[Marek Czernohous]

nouveau_channel_init() arms the channel-kill subscription early, right
after mapping userd, and only creates the fence context at the very end
of the same function. The handler it installs, nouveau_channel_killed(),
reaches nouveau_fence_context_kill(chan->fence).

The NULL check in nouveau_channel_kill() does not cover the window in
between. Every backend that can reach it publishes the pointer before the
context is usable:

	fctx = chan->fence = kzalloc_obj(*fctx);
	if (!fctx)
		return -ENOMEM;

	nouveau_fence_context_new(chan, &fctx->base);

and nouveau_fence_context_new() is what runs spin_lock_init(&fctx->lock)
and INIT_LIST_HEAD(&fctx->pending). An event arriving after the
assignment but before that call finds chan->fence non-NULL and unusable:
nouveau_fence_context_kill() takes a lock that was never initialised and
walks a list head whose next pointer is still the NULL left by kzalloc().

Give the fence context a ->ready flag and hand the kill over through it.
nouveau_fence_context_arm() sets the flag once nouveau_channel_init()
has finished building the context, and nouveau_channel_kill() leaves the
context alone until it is set. A kill arriving while the context is
still being built is no longer lost either: it is recorded in
chan->killed, and nouveau_fence_context_arm() acts on it as soon as
there is a context to kill.

The two sides hand over rather than exclude each other, because the kill
side must not touch fctx->lock at all before the context is built, which
is the very bug being fixed. Each stores its own flag before it loads
the other's, so at least one of them observes the other. Both observing
it is harmless: nouveau_fence_context_kill() then walks a list the first
caller has already emptied.

This does not close the other window. A kill delivered before
nouveau_channel_init() subscribes is still not observed at all, and
nvkm_uchan_init() makes the channel schedulable before that point.
Closing that one means subscribing before the channel becomes
schedulable, which is a larger change than this fix.

The approach is Lyude Paul's suggestion. It is implemented with two
differences from the sketch, both following from the same detail.

The sketch checks chan->killed before setting ->ready. Both sides have
to store their own flag before loading the other's, or the interleaving
loses the kill: arm() reads killed == 0, kill() sets killed and reads
ready == false, arm() then sets ready, and neither calls
nouveau_fence_context_kill(). That outcome is reachable under sequential
consistency, so no barrier can forbid it and the two accesses have to be
the other way round in program order. Swapped, and with the smp_mb() on
each side, this is the store-buffering pattern of
tools/memory-model/litmus-tests/SB+fencembonceonces.litmus.

The sketch also holds fctx->lock across the handover. The kill side
cannot join it, because reaching fctx->lock is exactly what has to be
avoided until the context is built: on those backends chan->fence is
published by the allocation, before nouveau_fence_context_new() calls
spin_lock_init().
So ->ready is read outside the lock. That answers the open question in
the sketch as well: it does not have to be atomic_t, but it does have to
be published with release and read with acquire, so that a caller that
sees it set also sees the initialised lock and list.

Fixes: ea13e5abf807 ("drm/nouveau: signal pending fences when channel has been killed")
Cc: stable@vger.kernel.org
Suggested-by: Lyude Paul <lyude@redhat.com>
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/nouveau_chan.c  | 19 ++++++++++++++++---
 drivers/gpu/drm/nouveau/nouveau_fence.c | 19 +++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_fence.h |  8 ++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
index f142f6310596..605ce74c0d15 100644
--- a/drivers/gpu/drm/nouveau/nouveau_chan.c
+++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
@@ -43,9 +43,17 @@ module_param_named(vram_pushbuf, nouveau_vram_pushbuf, int, 0400);
 void
 nouveau_channel_kill(struct nouveau_channel *chan)
 {
+	struct nouveau_fence_chan *fctx;
+
 	atomic_set(&chan->killed, 1);
-	if (chan->fence)
-		nouveau_fence_context_kill(chan->fence, -ENODEV);
+
+	/* Pairs with the smp_mb() in nouveau_fence_context_arm(). */
+	smp_mb();
+
+	fctx = READ_ONCE(chan->fence);
+	/* Pairs with the smp_store_release() there. */
+	if (fctx && smp_load_acquire(&fctx->ready))
+		nouveau_fence_context_kill(fctx, -ENODEV);
 }
 
 static int
@@ -494,7 +502,12 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
 	}
 
 	/* initialise synchronisation */
-	return nouveau_fence(drm)->context_new(chan);
+	ret = nouveau_fence(drm)->context_new(chan);
+	if (ret)
+		return ret;
+
+	nouveau_fence_context_arm(chan);
+	return 0;
 }
 
 int
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c
index edbe9e08ba0f..2fed631d44ba 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -93,6 +93,25 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error)
 	spin_unlock_irqrestore(&fctx->lock, flags);
 }
 
+/*
+ * Declare a finished fence context killable.  A kill can arrive while the
+ * caller is still building the context, so this and nouveau_channel_kill()
+ * hand over through fctx->ready and chan->killed.
+ */
+void
+nouveau_fence_context_arm(struct nouveau_channel *chan)
+{
+	struct nouveau_fence_chan *fctx = chan->fence;
+
+	/* Pairs with the smp_load_acquire() in nouveau_channel_kill(). */
+	smp_store_release(&fctx->ready, true);
+	/* Pairs with the smp_mb() there: store-buffering, one side always sees the other. */
+	smp_mb();
+
+	if (atomic_read(&chan->killed))
+		nouveau_fence_context_kill(fctx, -ENODEV);
+}
+
 void
 nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h
index 183dd43ecfff..d9fede5dcba6 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.h
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.h
@@ -53,6 +53,13 @@ struct nouveau_fence_chan {
 	struct work_struct uevent_work;
 	struct nvif_event event;
 	int notify_ref, dead, killed;
+
+	/*
+	 * Set by nouveau_fence_context_arm() once the context is complete.
+	 * Read without fctx->lock, which nouveau_channel_kill() may not
+	 * touch until it is set.
+	 */
+	bool ready;
 };
 
 struct nouveau_fence_priv {
@@ -71,6 +78,7 @@ void nouveau_fence_context_new(struct nouveau_channel *, struct nouveau_fence_ch
 void nouveau_fence_context_del(struct nouveau_fence_chan *);
 void nouveau_fence_context_free(struct nouveau_fence_chan *);
 void nouveau_fence_context_kill(struct nouveau_fence_chan *, int error);
+void nouveau_fence_context_arm(struct nouveau_channel *chan);
 
 int nv04_fence_create(struct nouveau_drm *);
 int nv04_fence_mthd(struct nouveau_channel *, u32, u32, u32);
-- 
2.54.0

[PATCH v4 3/3] drm/nouveau: subscribe to channel-kill events on NV50 and newer

[Marek Czernohous]

nouveau_channel_init() only subscribes to the channel-killed event for
FERMI_CHANNEL_GPFIFO and newer. On NV50/Tesla the subscription therefore
never happens, and nvkm_chan_error()'s NVKM_CHAN_EVENT_ERRORED is
delivered into an empty notifier list.

Today that is harmless, because nothing kills a channel on Tesla: the
only nvkm_chan_error() callers are the Fermi and newer recovery paths.
So this patch changes no observable behaviour on its own, and that is
deliberate: it removes a latent trap before anything can fall into it.
I am carrying a Tesla recovery path that does add such a caller and will
send it separately once it is ready. Without a subscriber in place the
consequences there are severe: nouveau_channel_killed() never runs, so
nouveau_fence_context_kill() never runs either, and the pending fences
of the killed channel are never signalled. Everything waiting on them
waits forever: drm_atomic_helper_wait_for_fences() in the display commit
tail waits uninterruptibly and without a timeout, and the TTM delayed
delete workers wait in TASK_UNINTERRUPTIBLE. The user sees a frozen
desktop on a machine that is otherwise alive, and nothing in the kernel
ends that state: both waits pass MAX_SCHEDULE_TIMEOUT, so the fences
cannot time out. They are signalled only when the fence context is torn
down, that is when the DRM client owning the channel closes its fd and
nouveau_fence_context_del() runs. Killing the client, or rebooting,
clears it; waiting does not.
That is also a dma-fence contract violation: a fence must always be
signalled, with an error if necessary.

Lower the class gate to NV50_CHANNEL_GPFIFO. The nvkm side is already
class neutral: the KILLED case hangs the notifier on runl->chid->event,
which every fifo owns since the runlist rework, and nvkm_uchan_uevent()
does not discriminate by class. Pre-NV50 chips keep the old behaviour,
so NV04 to NV40 are unaffected.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/nouveau_chan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
index 605ce74c0d15..54e2202cb852 100644
--- a/drivers/gpu/drm/nouveau/nouveau_chan.c
+++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
@@ -378,7 +378,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
 	if (ret)
 		return ret;
 
-	if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
+	if (chan->user.oclass >= NV50_CHANNEL_GPFIFO) {
 		DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
 				sizeof(struct nvif_chan_event_v0));
 		struct nvif_chan_event_v0 *host =
-- 
2.54.0