[PATCH v4 0/2] drm/nouveau: NVAC (MCP79) stability: MSI rearm and a NULL crtc guard

[PATCH v4 0/2] drm/nouveau: NVAC (MCP79) stability: MSI rearm and a NULL crtc guard

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

This is v4 of two NVAC (MCP79) stability fixes.  v3 went out on 11 June
and has had no replies:

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

Nothing in either patch changed.  Same diffs, same reasoning, same
trailers.  The reason this is a v4 and not a ping is that 2/2 no longer
applies to mainline, so a ping would have pointed at a series nobody can
test.

What moved was somebody else's context, not this code.
Commit 5164f7e7ff8e ("drm: Rename struct drm_atomic_state to
drm_atomic_commit") changed the callback signature, so
nv50_sor_atomic_disable() now takes a struct drm_atomic_commit *state.
2/2 is rebased onto that by hand.  1/2 applies unchanged and is
byte-identical to v3.

Both bugs are still live in c21bb4193868.  dispnv50/disp.c:1568 still
does nv50_head(nv_encoder->crtc) with no NULL check, and there is no
guard anywhere in the function.  Immediately below it, :1570 takes
nv50_outp_get_old_connector() and :1572 dereferences the result without
checking it.

1/2 drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)

    NVAC re-arms MSI through the MMIO mirror of PCI config space, which
    is unreliable on this IGP.  A missed re-arm leaves the interrupt
    line dead, submission times out and the GPU looks hung until
    reboot.  Give NVAC its own pci func that re-arms through real
    config space, the way commit 5112abc6a433 ("drm/nouveau/pci/g92:
    Fix rearm") already did for g92.  Only NVAC is switched, because
    that is the part this was validated on.

2/2 drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

    Restore the NULL check that
    commit f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove
    (nv_encoder->crtc) checks in ->disable callbacks") removed, as a
    drm_WARN_ON_ONCE() rather than a silent return, since a NULL crtc
    here is a state-tracking inconsistency worth seeing.  The guard is
    hoisted above the backlight teardown so the old connector is only
    used after it has been checked; without that the oops would just
    move a few lines down.

Testing.  The MSI change continues to run here on the Mac mini as a
daily driver, and Fab Stz confirmed it independently on an iMac9,1.
Both Tested-by tags are carried over unchanged from v3; the
diffs they were given are the diffs sent here, apart from the mechanical
rebase of 2/2.  For v4 I rebuilt 2/2 with CONFIG_DRM_NOUVEAU_BACKLIGHT
set to y and to n, because the patch splits the #ifdef block in two and
that is where a change of this shape tends to break.

If something about the shape of the series is what is holding it up, I
would rather hear that than keep rebasing it.

Marek Czernohous (2):
  drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)
  drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

 drivers/gpu/drm/nouveau/dispnv50/disp.c       | 30 +++++++++++++---
 .../gpu/drm/nouveau/include/nvkm/subdev/pci.h |  1 +
 .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
 .../gpu/drm/nouveau/nvkm/subdev/pci/Kbuild    |  1 +
 .../gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c   | 35 +++++++++++++++++++
 5 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c


base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.54.0

[PATCH v4 0/2] drm/nouveau: NVAC (MCP79) stability: MSI rearm and a NULL crtc guard

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

This is v4 of two NVAC (MCP79) stability fixes.  v3 went out on 11 June
and has had no replies:

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

Nothing in either patch changed.  Same diffs, same reasoning, same
trailers.  The reason this is a v4 and not a ping is that 2/2 no longer
applies to mainline, so a ping would have pointed at a series nobody can
test.

What moved was somebody else's context, not this code.
Commit 5164f7e7ff8e ("drm: Rename struct drm_atomic_state to
drm_atomic_commit") changed the callback signature, so
nv50_sor_atomic_disable() now takes a struct drm_atomic_commit *state.
2/2 is rebased onto that by hand.  1/2 applies unchanged and is
byte-identical to v3.

Both bugs are still live in c21bb4193868.  dispnv50/disp.c:1568 still
does nv50_head(nv_encoder->crtc) with no NULL check, and there is no
guard anywhere in the function.  Immediately below it, :1570 takes
nv50_outp_get_old_connector() and :1572 dereferences the result without
checking it.

1/2 drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)

    NVAC re-arms MSI through the MMIO mirror of PCI config space, which
    is unreliable on this IGP.  A missed re-arm leaves the interrupt
    line dead, submission times out and the GPU looks hung until
    reboot.  Give NVAC its own pci func that re-arms through real
    config space, the way commit 5112abc6a433 ("drm/nouveau/pci/g92:
    Fix rearm") already did for g92.  Only NVAC is switched, because
    that is the part this was validated on.

2/2 drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

    Restore the NULL check that
    commit f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove
    (nv_encoder->crtc) checks in ->disable callbacks") removed, as a
    drm_WARN_ON_ONCE() rather than a silent return, since a NULL crtc
    here is a state-tracking inconsistency worth seeing.  The guard is
    hoisted above the backlight teardown so the old connector is only
    used after it has been checked; without that the oops would just
    move a few lines down.

Testing.  The MSI change continues to run here on the Mac mini as a
daily driver, and Fab Stz confirmed it independently on an iMac9,1.
Both Tested-by tags are carried over unchanged from v3; the
diffs they were given are the diffs sent here, apart from the mechanical
rebase of 2/2.  For v4 I rebuilt 2/2 with CONFIG_DRM_NOUVEAU_BACKLIGHT
set to y and to n, because the patch splits the #ifdef block in two and
that is where a change of this shape tends to break.

If something about the shape of the series is what is holding it up, I
would rather hear that than keep rebasing it.

Marek Czernohous (2):
  drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)
  drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

 drivers/gpu/drm/nouveau/dispnv50/disp.c       | 30 +++++++++++++---
 .../gpu/drm/nouveau/include/nvkm/subdev/pci.h |  1 +
 .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
 .../gpu/drm/nouveau/nvkm/subdev/pci/Kbuild    |  1 +
 .../gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c   | 35 +++++++++++++++++++
 5 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c


base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.54.0

[PATCH v4 1/2] drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

NVAC (MCP79/MCP7A) uses g94_pci_func, whose .msi_rearm is
nv40_pci_msi_rearm(): a re-arm write through the MMIO mirror of PCI
config space.  On this IGP that path is unreliable; when a re-arm is
missed the interrupt line stays dead, command submission times out and
the GPU appears hung until reboot.  On an Apple Mac mini (early 2009,
MCP79, boot0 0x0ac080b1) this showed as sporadic fifo timeouts and GPU
hangs under load unless MSI was disabled via config=NvMSI=0.

Give NVAC its own pci func that re-arms through real PCI config space
(nv46_pci_msi_rearm) instead.  This follows existing precedent: nv46.c
documents the MMIO-mirror re-arm as broken on several related parts,
and commit 5112abc6a433 ("drm/nouveau/pci/g92: Fix rearm") fixed g92
the same way while moving the remaining chipsets, NVAC included, into
the newly added shared g94 table, where NVAC stayed on the MMIO path.
This change completes that fix for NVAC.  The sibling IGP NVAA
(MCP77/MCP78) has MSI disabled entirely as "reported broken" in
nvkm_pci_new_(); NVAC works correctly once the re-arm goes through
config space, so disabling MSI is not necessary.

Only NVAC is switched: that is the hardware this has been validated
on.  The other users of g94_pci_func (G94/G96/G98/GT2xx and the
MCP77/MCP89 IGPs) keep their current behavior; MCP77 and MCP89
plausibly want the same treatment but were not tested.

Tested on the Mac mini as a daily driver for two months with MSI
enabled and zero fifo timeouts.  Independently confirmed stable on an
iMac9,1 (MCP79) running 6.12.90 with the v1 form of this change (the
same one-line functional switch, applied to that kernel's g94
implementation).

Fixes: 5112abc6a433 ("drm/nouveau/pci/g92: Fix rearm")
Cc: <stable@vger.kernel.org> # v6.16+
Tested-by: Fab Stz <fabstz-it@yahoo.fr>
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 .../gpu/drm/nouveau/include/nvkm/subdev/pci.h |  1 +
 .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
 .../gpu/drm/nouveau/nvkm/subdev/pci/Kbuild    |  1 +
 .../gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c   | 35 +++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c

diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
index 112b674ed9c8..0172e0d200f8 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
@@ -46,6 +46,7 @@ int nv4c_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct n
 int g84_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int g92_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int g94_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
+int mcp79_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gf100_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gf106_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gk104_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
index ea62dc97f118..f7e02a16e5bc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
@@ -1237,7 +1237,7 @@ nvac_chipset = {
 	.mc       = { 0x00000001, g98_mc_new },
 	.mmu      = { 0x00000001, mcp77_mmu_new },
 	.mxm      = { 0x00000001, nv50_mxm_new },
-	.pci      = { 0x00000001, g94_pci_new },
+	.pci      = { 0x00000001, mcp79_pci_new },
 	.therm    = { 0x00000001, g84_therm_new },
 	.timer    = { 0x00000001, nv41_timer_new },
 	.volt     = { 0x00000001, nv40_volt_new },
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
index a14ea0f7b1c8..90f03baeb22a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
@@ -9,6 +9,7 @@ nvkm-y += nvkm/subdev/pci/nv4c.o
 nvkm-y += nvkm/subdev/pci/g84.o
 nvkm-y += nvkm/subdev/pci/g92.o
 nvkm-y += nvkm/subdev/pci/g94.o
+nvkm-y += nvkm/subdev/pci/mcp79.o
 nvkm-y += nvkm/subdev/pci/gf100.o
 nvkm-y += nvkm/subdev/pci/gf106.o
 nvkm-y += nvkm/subdev/pci/gk104.o
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c
new file mode 100644
index 000000000000..e2ae242f1aa7
--- /dev/null
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2026 Marek Czernohous
+ *
+ * MCP79/MCP7A (NVAC): like g94, but MSI re-arm goes through real PCI
+ * config space.  The MMIO-mirror re-arm is unreliable on this IGP and a
+ * missed re-arm kills the interrupt line (see the nv46 comment; g92
+ * already re-arms through config space for the same reason).
+ */
+#include "priv.h"
+
+static const struct nvkm_pci_func
+mcp79_pci_func = {
+	.cfg = { .addr = 0x088000, .size = 0x1000 },
+
+	.init = g84_pci_init,
+	.msi_rearm = nv46_pci_msi_rearm,
+
+	.pcie.init = g84_pcie_init,
+	.pcie.set_link = g84_pcie_set_link,
+
+	.pcie.max_speed = g84_pcie_max_speed,
+	.pcie.cur_speed = g84_pcie_cur_speed,
+
+	.pcie.set_version = g84_pcie_set_version,
+	.pcie.version = g84_pcie_version,
+	.pcie.version_supported = g92_pcie_version_supported,
+};
+
+int
+mcp79_pci_new(struct nvkm_device *device, enum nvkm_subdev_type type, int inst,
+	      struct nvkm_pci **ppci)
+{
+	return nvkm_pci_new_(&mcp79_pci_func, device, type, inst, ppci);
+}
-- 
2.54.0

[PATCH v4 1/2] drm/nouveau/pci: use config-space MSI rearm on MCP79/MCP7A (NVAC)

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

NVAC (MCP79/MCP7A) uses g94_pci_func, whose .msi_rearm is
nv40_pci_msi_rearm(): a re-arm write through the MMIO mirror of PCI
config space.  On this IGP that path is unreliable; when a re-arm is
missed the interrupt line stays dead, command submission times out and
the GPU appears hung until reboot.  On an Apple Mac mini (early 2009,
MCP79, boot0 0x0ac080b1) this showed as sporadic fifo timeouts and GPU
hangs under load unless MSI was disabled via config=NvMSI=0.

Give NVAC its own pci func that re-arms through real PCI config space
(nv46_pci_msi_rearm) instead.  This follows existing precedent: nv46.c
documents the MMIO-mirror re-arm as broken on several related parts,
and commit 5112abc6a433 ("drm/nouveau/pci/g92: Fix rearm") fixed g92
the same way while moving the remaining chipsets, NVAC included, into
the newly added shared g94 table, where NVAC stayed on the MMIO path.
This change completes that fix for NVAC.  The sibling IGP NVAA
(MCP77/MCP78) has MSI disabled entirely as "reported broken" in
nvkm_pci_new_(); NVAC works correctly once the re-arm goes through
config space, so disabling MSI is not necessary.

Only NVAC is switched: that is the hardware this has been validated
on.  The other users of g94_pci_func (G94/G96/G98/GT2xx and the
MCP77/MCP89 IGPs) keep their current behavior; MCP77 and MCP89
plausibly want the same treatment but were not tested.

Tested on the Mac mini as a daily driver for two months with MSI
enabled and zero fifo timeouts.  Independently confirmed stable on an
iMac9,1 (MCP79) running 6.12.90 with the v1 form of this change (the
same one-line functional switch, applied to that kernel's g94
implementation).

Fixes: 5112abc6a433 ("drm/nouveau/pci/g92: Fix rearm")
Cc: <stable@vger.kernel.org> # v6.16+
Tested-by: Fab Stz <fabstz-it@yahoo.fr>
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 .../gpu/drm/nouveau/include/nvkm/subdev/pci.h |  1 +
 .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
 .../gpu/drm/nouveau/nvkm/subdev/pci/Kbuild    |  1 +
 .../gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c   | 35 +++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c

diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
index 112b674ed9c8..0172e0d200f8 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/pci.h
@@ -46,6 +46,7 @@ int nv4c_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct n
 int g84_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int g92_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int g94_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
+int mcp79_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gf100_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gf106_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
 int gk104_pci_new(struct nvkm_device *, enum nvkm_subdev_type, int inst, struct nvkm_pci **);
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
index ea62dc97f118..f7e02a16e5bc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
@@ -1237,7 +1237,7 @@ nvac_chipset = {
 	.mc       = { 0x00000001, g98_mc_new },
 	.mmu      = { 0x00000001, mcp77_mmu_new },
 	.mxm      = { 0x00000001, nv50_mxm_new },
-	.pci      = { 0x00000001, g94_pci_new },
+	.pci      = { 0x00000001, mcp79_pci_new },
 	.therm    = { 0x00000001, g84_therm_new },
 	.timer    = { 0x00000001, nv41_timer_new },
 	.volt     = { 0x00000001, nv40_volt_new },
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
index a14ea0f7b1c8..90f03baeb22a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/Kbuild
@@ -9,6 +9,7 @@ nvkm-y += nvkm/subdev/pci/nv4c.o
 nvkm-y += nvkm/subdev/pci/g84.o
 nvkm-y += nvkm/subdev/pci/g92.o
 nvkm-y += nvkm/subdev/pci/g94.o
+nvkm-y += nvkm/subdev/pci/mcp79.o
 nvkm-y += nvkm/subdev/pci/gf100.o
 nvkm-y += nvkm/subdev/pci/gf106.o
 nvkm-y += nvkm/subdev/pci/gk104.o
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c
new file mode 100644
index 000000000000..e2ae242f1aa7
--- /dev/null
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/pci/mcp79.c
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2026 Marek Czernohous
+ *
+ * MCP79/MCP7A (NVAC): like g94, but MSI re-arm goes through real PCI
+ * config space.  The MMIO-mirror re-arm is unreliable on this IGP and a
+ * missed re-arm kills the interrupt line (see the nv46 comment; g92
+ * already re-arms through config space for the same reason).
+ */
+#include "priv.h"
+
+static const struct nvkm_pci_func
+mcp79_pci_func = {
+	.cfg = { .addr = 0x088000, .size = 0x1000 },
+
+	.init = g84_pci_init,
+	.msi_rearm = nv46_pci_msi_rearm,
+
+	.pcie.init = g84_pcie_init,
+	.pcie.set_link = g84_pcie_set_link,
+
+	.pcie.max_speed = g84_pcie_max_speed,
+	.pcie.cur_speed = g84_pcie_cur_speed,
+
+	.pcie.set_version = g84_pcie_set_version,
+	.pcie.version = g84_pcie_version,
+	.pcie.version_supported = g92_pcie_version_supported,
+};
+
+int
+mcp79_pci_new(struct nvkm_device *device, enum nvkm_subdev_type type, int inst,
+	      struct nvkm_pci **ppci)
+{
+	return nvkm_pci_new_(&mcp79_pci_func, device, type, inst, ppci);
+}
-- 
2.54.0

[PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nv50_sor_atomic_disable() unconditionally computes
nv50_head(nv_encoder->crtc) and dereferences the result a few lines
later.  nv_encoder->crtc is nouveau's own shadow pointer, set in
.atomic_enable and cleared at the end of .atomic_disable.
Commit f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc)
checks in ->disable callbacks") removed the NULL check here,
reasoning that the atomic helpers never call ->disable without a
crtc.  On NVAC (MCP79) under Wayland sessions (observed with Weston's
DRM backend and with labwc/wlroots) we have hit the NULL case in
practice during session teardown and VT switches: disable runs without
(or after) its matching enable, and because nv50_head() is
container_of(), the NULL does not stay NULL but becomes a bogus
non-NULL pointer, so the subsequent head dereferences fault and the
kernel oopses.

Restore the guard, as drm_WARN_ON_ONCE() instead of a silent return:
a NULL crtc here still indicates a state-tracking inconsistency that
should stay visible.  Return without touching the output; in this path
either enable never ran (nothing to tear down) or an earlier disable
already did the teardown, and the encoder release is handled by the
commit_tail release loop in both cases.  (That loop then rejects the
release of a never-acquired output with -EINVAL in the nvif layer,
which is harmless; the vanilla code oopsed before ever reaching it.)

The same inconsistent-state path can also leave the encoder without an
old connector state, in which case nv50_outp_get_old_connector()
returns NULL while the backlight teardown dereferenced it
unconditionally, so the oops would only have moved there.  Hoist the
guard above all of that and look at the old connector only after
checking it.

Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org>
Tested-by: Fab Stz <fabstz-it@yahoo.fr>
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 30 ++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 364227f5456f..f532b0ed8880 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1565,16 +1565,36 @@ static void
 nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
 {
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
-	struct nv50_head *head = nv50_head(nv_encoder->crtc);
+	struct nv50_head *head;
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
-	struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
+	struct nouveau_connector *nv_connector;
 	struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
-	struct nouveau_backlight *backlight = nv_connector->backlight;
-	struct drm_dp_aux *aux = &nv_connector->aux;
+	struct nouveau_backlight *backlight;
 	int ret;
+#endif
 
+	/* nv_encoder->crtc is the driver's shadow pointer, set in
+	 * .atomic_enable (and by the boot-time hardware readback) and
+	 * cleared at the end of this function.  NULL here
+	 * means disable-without-enable or a double disable; bail before
+	 * container_of() turns it into a bogus head pointer (checking the
+	 * result would not work, container_of(NULL) is never NULL).  The
+	 * encoder release is handled by the commit_tail release loop, so
+	 * there is nothing to clean up here.
+	 */
+	if (drm_WARN_ON_ONCE(encoder->dev, !nv_encoder->crtc))
+		return;
+	head = nv50_head(nv_encoder->crtc);
+
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
+	/* The same inconsistent-state path can leave us without an old
+	 * connector state, so check before touching it.
+	 */
+	nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
+	backlight = nv_connector ? nv_connector->backlight : NULL;
 	if (backlight && backlight->uses_dpcd) {
-		ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
+		ret = drm_edp_backlight_disable(&nv_connector->aux,
+						&backlight->edp_info);
 		if (ret < 0)
 			NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n",
 				 nv_connector->base.base.id, nv_connector->base.name, ret);
-- 
2.54.0

[PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nv50_sor_atomic_disable() unconditionally computes
nv50_head(nv_encoder->crtc) and dereferences the result a few lines
later.  nv_encoder->crtc is nouveau's own shadow pointer, set in
.atomic_enable and cleared at the end of .atomic_disable.
Commit f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc)
checks in ->disable callbacks") removed the NULL check here,
reasoning that the atomic helpers never call ->disable without a
crtc.  On NVAC (MCP79) under Wayland sessions (observed with Weston's
DRM backend and with labwc/wlroots) we have hit the NULL case in
practice during session teardown and VT switches: disable runs without
(or after) its matching enable, and because nv50_head() is
container_of(), the NULL does not stay NULL but becomes a bogus
non-NULL pointer, so the subsequent head dereferences fault and the
kernel oopses.

Restore the guard, as drm_WARN_ON_ONCE() instead of a silent return:
a NULL crtc here still indicates a state-tracking inconsistency that
should stay visible.  Return without touching the output; in this path
either enable never ran (nothing to tear down) or an earlier disable
already did the teardown, and the encoder release is handled by the
commit_tail release loop in both cases.  (That loop then rejects the
release of a never-acquired output with -EINVAL in the nvif layer,
which is harmless; the vanilla code oopsed before ever reaching it.)

The same inconsistent-state path can also leave the encoder without an
old connector state, in which case nv50_outp_get_old_connector()
returns NULL while the backlight teardown dereferenced it
unconditionally, so the oops would only have moved there.  Hoist the
guard above all of that and look at the old connector only after
checking it.

Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org>
Tested-by: Fab Stz <fabstz-it@yahoo.fr>
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 30 ++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 364227f5456f..f532b0ed8880 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1565,16 +1565,36 @@ static void
 nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
 {
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
-	struct nv50_head *head = nv50_head(nv_encoder->crtc);
+	struct nv50_head *head;
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
-	struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
+	struct nouveau_connector *nv_connector;
 	struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
-	struct nouveau_backlight *backlight = nv_connector->backlight;
-	struct drm_dp_aux *aux = &nv_connector->aux;
+	struct nouveau_backlight *backlight;
 	int ret;
+#endif
 
+	/* nv_encoder->crtc is the driver's shadow pointer, set in
+	 * .atomic_enable (and by the boot-time hardware readback) and
+	 * cleared at the end of this function.  NULL here
+	 * means disable-without-enable or a double disable; bail before
+	 * container_of() turns it into a bogus head pointer (checking the
+	 * result would not work, container_of(NULL) is never NULL).  The
+	 * encoder release is handled by the commit_tail release loop, so
+	 * there is nothing to clean up here.
+	 */
+	if (drm_WARN_ON_ONCE(encoder->dev, !nv_encoder->crtc))
+		return;
+	head = nv50_head(nv_encoder->crtc);
+
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
+	/* The same inconsistent-state path can leave us without an old
+	 * connector state, so check before touching it.
+	 */
+	nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
+	backlight = nv_connector ? nv_connector->backlight : NULL;
 	if (backlight && backlight->uses_dpcd) {
-		ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
+		ret = drm_edp_backlight_disable(&nv_connector->aux,
+						&backlight->edp_info);
 		if (ret < 0)
 			NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n",
 				 nv_connector->base.base.id, nv_connector->base.name, ret);
-- 
2.54.0

Re: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch adds a guard against disable-without-enable or double-disable in `nv50_sor_atomic_disable()`, but misses an identical structural vulnerability in `nv50_msto_atomic_disable()`.
--

--- Patch [2]: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable() ---
commit 7ccc8c2fed1546597f7a2ed45ab47da4012b6d66
Author: Marek Czernohous <marek@czernohous.de>

drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

nv50_sor_atomic_disable() unconditionally computes
nv50_head(nv_encoder->crtc) and dereferences the result a few lines
later. nv_encoder->crtc is nouveau's own shadow pointer, set in
.atomic_enable and cleared at the end of .atomic_disable.
This patch restores the guard, as drm_WARN_ON_ONCE() instead of a
silent return.

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 364227f5456f1..f532b0ed88809 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1565,16 +1565,36 @@ static void
>  nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
>  {
>  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
> -	struct nv50_head *head = nv50_head(nv_encoder->crtc);
> +	struct nv50_head *head;
>  #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the
disable-without-enable issue here, does a similar vulnerability
exist in nv50_msto_atomic_disable() in the same file?

Looking at nv50_msto_atomic_disable():

static void
nv50_msto_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
{
	struct nv50_msto *msto = nv50_msto(encoder);
	struct nv50_mstc *mstc = msto->mstc;
	struct nv50_mstm *mstm = mstc->mstm;

If this is called during session teardown without a matching
.atomic_enable, couldn't msto->mstc be NULL, leading to a NULL pointer
dereference when accessing mstc->mstm?

> -	struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
>  	struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
> -	struct nouveau_backlight *backlight = nv_connector->backlight;
> -	struct drm_dp_aux *aux = &nv_connector->aux;
>  	int ret;
> +#endif
>  
> +	/* nv_encoder->crtc is the driver's shadow pointer, set in
> +	 * .atomic_enable (and by the boot-time hardware readback) and
> +	 * cleared at the end of this function.  NULL here
> +	 * means disable-without-enable or a double disable; bail before
> +	 * container_of() turns it into a bogus head pointer (checking the
> +	 * result would not work, container_of(NULL) is never NULL).  The
> +	 * encoder release is handled by the commit_tail release loop, so
> +	 * there is nothing to clean up here.
> +	 */
> +	if (drm_WARN_ON_ONCE(encoder->dev, !nv_encoder->crtc))
> +		return;
> +	head = nv50_head(nv_encoder->crtc);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178688574400.522643.6695278742335367229@gmail.com?part=2

Re: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[Marek Czernohous]

> This isn't a bug introduced by this patch, but while fixing the
> disable-without-enable issue here, does a similar vulnerability
> exist in nv50_msto_atomic_disable() in the same file?
[...]
> If this is called during session teardown without a matching
> .atomic_enable, couldn't msto->mstc be NULL, leading to a NULL
> pointer dereference when accessing mstc->mstm?

The shape is the same.  nv50_msto_atomic_disable() takes msto->mstc
without checking it (dispnv50/disp.c:1079-1080):

	struct nv50_mstc *mstc = msto->mstc;
	struct nv50_mstm *mstm = mstc->mstm;

and the pointer can hold NULL: it is assigned only in
nv50_msto_atomic_enable() (:1070) and set back to NULL in
nv50_msto_cleanup() (:918).

What I could not establish is that the callback is reached in that
state.  nv50 does not drive the encoder disable from the atomic
helpers, it drives it from its own outp list in
nv50_disp_atomic_commit_tail() (:2229 to :2240), and outp->clr.ctrl
is only set in nv50_disp_outp_atomic_check_clr() (:2530), behind two
conditions: the connector sat on a CRTC in the old state (:2517), and
that CRTC was active in the old state (:2522).  A CRTC that was
active came up through a commit that ran .atomic_enable (:2272 to
:2274), which is where msto->mstc is assigned.  I did not find a way
around that, so I cannot claim that a disable with no matching enable
gets there.

The one path I could not rule out is the early return in
nv50_msto_atomic_enable():

	if (WARN_ON(!mstc))
		return;

at :1049.  It returns before the assignment at :1070, while
commit_tail still sets outp->enabled = true at :2274.  That sits
behind a WARN_ON, so it is a second-order path rather than a fresh
bug.

For completeness, the other two places that read msto->mstc without
a check, nv50_msto_cleanup() (:902 and :906 to :908) and
nv50_msto_prepare() (:934), are covered by their callers, which test
"mstc && mstc->mstm == mstm" at :1318, :1347 and :1359.
nv50_real_outp() checks for itself at :889.  The disable callback is
the only reader left without a check.

I am not adding a patch for it to this series, for the same reason
2/2 is scoped the way it is: 2/2 fixes something I hit on real
hardware and can reproduce.  This is MST, I have no MST setup here,
and a guard written against a path I cannot exercise is a guess.  If
the maintainers want it anyway I will send it as a separate patch,
but I would rather hear from someone who can run MST whether that
callback is reachable with msto->mstc NULL at all.

v4 stands as posted, no respin planned for this.  The question is
orthogonal to both patches.

Re: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

[Marek Czernohous]

> This isn't a bug introduced by this patch, but while fixing the
> disable-without-enable issue here, does a similar vulnerability
> exist in nv50_msto_atomic_disable() in the same file?
[...]
> If this is called during session teardown without a matching
> .atomic_enable, couldn't msto->mstc be NULL, leading to a NULL
> pointer dereference when accessing mstc->mstm?

The shape is the same.  nv50_msto_atomic_disable() takes msto->mstc
without checking it (dispnv50/disp.c:1079-1080):

	struct nv50_mstc *mstc = msto->mstc;
	struct nv50_mstm *mstm = mstc->mstm;

and the pointer can hold NULL: it is assigned only in
nv50_msto_atomic_enable() (:1070) and set back to NULL in
nv50_msto_cleanup() (:918).

What I could not establish is that the callback is reached in that
state.  nv50 does not drive the encoder disable from the atomic
helpers, it drives it from its own outp list in
nv50_disp_atomic_commit_tail() (:2229 to :2240), and outp->clr.ctrl
is only set in nv50_disp_outp_atomic_check_clr() (:2530), behind two
conditions: the connector sat on a CRTC in the old state (:2517), and
that CRTC was active in the old state (:2522).  A CRTC that was
active came up through a commit that ran .atomic_enable (:2272 to
:2274), which is where msto->mstc is assigned.  I did not find a way
around that, so I cannot claim that a disable with no matching enable
gets there.

The one path I could not rule out is the early return in
nv50_msto_atomic_enable():

	if (WARN_ON(!mstc))
		return;

at :1049.  It returns before the assignment at :1070, while
commit_tail still sets outp->enabled = true at :2274.  That sits
behind a WARN_ON, so it is a second-order path rather than a fresh
bug.

For completeness, the other two places that read msto->mstc without
a check, nv50_msto_cleanup() (:902 and :906 to :908) and
nv50_msto_prepare() (:934), are covered by their callers, which test
"mstc && mstc->mstm == mstm" at :1318, :1347 and :1359.
nv50_real_outp() checks for itself at :889.  The disable callback is
the only reader left without a check.

I am not adding a patch for it to this series, for the same reason
2/2 is scoped the way it is: 2/2 fixes something I hit on real
hardware and can reproduce.  This is MST, I have no MST setup here,
and a guard written against a path I cannot exercise is a guess.  If
the maintainers want it anyway I will send it as a separate patch,
but I would rather hear from someone who can run MST whether that
callback is reachable with msto->mstc NULL at all.

v4 stands as posted, no respin planned for this.  The question is
orthogonal to both patches.