[PATCH] drm/nouveau: handle nouveau_hw_get_clock() return value

[PATCH] drm/nouveau: handle nouveau_hw_get_clock() return value

[Bulatov Mark]

From: Mark Bulatov <ms.bulatov7@gmail.com>

In the nv04_update_arb() function, MClk and NVClk are passed to
nv04_calc_arb() or nv10_calc_arb(), where they are used as divisors.
MClk and NVClk are calculated using nouveau_hw_get_clock(), which, via
nouveau_hw_pllvals_to_clk(), can return 0, resulting in division by 0.

The result of nouveau_hw_get_pllvals() in the nouveau_hw_get_clock()
function can also return -ENOENT, which is not checked anywhere.

Fix all this by adding a check.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 6ee738610f41 ("drm/nouveau: Add DRM driver for NVIDIA GPUs")
Signed-off-by: Mark Bulatov <ms.bulatov7@gmail.com>
---
 drivers/gpu/drm/nouveau/dispnv04/arb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/arb.c b/drivers/gpu/drm/nouveau/dispnv04/arb.c
index 1d3542d6006b..2f59b8739fc1 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/arb.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/arb.c
@@ -202,6 +202,9 @@ nv04_update_arb(struct drm_device *dev, int VClk, int bpp,
 	uint32_t cfg1 = nvif_rd32(device, NV04_PFB_CFG1);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
+	if (MClk <= 0 || NVClk <= 0)
+		return;
+
 	sim_data.pclk_khz = VClk;
 	sim_data.mclk_khz = MClk;
 	sim_data.nvclk_khz = NVClk;
-- 
2.39.5

Claude review: drm/nouveau: handle nouveau_hw_get_clock() return value

[Claude Code Review Bot]

Overall Series Review

Subject: drm/nouveau: handle nouveau_hw_get_clock() return value
Author: Bulatov Mark <ms.bulatov7@gmail.com>
Patches: 1
Reviewed: 2026-05-16T10:30:37.685317

---

This is a single-patch series addressing a potential division-by-zero in the nouveau NV04 display arbitration code. The problem is real: `nouveau_hw_get_clock()` can return 0 (when `nouveau_hw_pllvals_to_clk()` gets zero M1/M2 divisors) or a negative error code (when `nouveau_hw_get_pllvals()` fails with `-ENOENT`). These values are then used as divisors throughout `nv04_calc_arb()` and `nv10_calc_arb()`, so the defensive check is warranted.

However, the fix has a significant problem: it introduces a **silent early return that leaves the caller's output parameters (`*burst` and `*lwm`) uninitialized**. The caller in `crtc.c:893` declares `arb_burst` and `arb_lwm` as bare `int` locals (line 848) without initialization, then immediately writes them to CRTC registers after the call. A silent return turns a division-by-zero into use-of-uninitialized-stack-data written to hardware registers — arguably trading one bug for another.

**Verdict: Needs revision.** The issue being fixed is real, but the fix needs to either (a) initialize the output parameters to safe defaults before returning, or (b) propagate an error code so the caller can skip the register writes.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/nouveau: handle nouveau_hw_get_clock() return value

[Claude Code Review Bot]

Patch Review

**Correctness concern — uninitialized output parameters on early return:**

```c
if (MClk <= 0 || NVClk <= 0)
    return;
```

When this early return is taken, `nv04_update_arb()` never writes to `*burst` or `*lwm`. The only caller path through `nouveau_calc_arb()` reaching this code is in `crtc.c`:

```c
int arb_burst, arb_lwm;            /* line 848 — uninitialized */
nouveau_calc_arb(dev, crtc->mode.clock, drm_fb->format->cpp[0] * 8,
                 &arb_burst, &arb_lwm);
regp->CRTC[NV_CIO_CRE_FF_INDEX] = arb_burst;       /* used uninitialized */
regp->CRTC[NV_CIO_CRE_FFLWM__INDEX] = arb_lwm & 0xff;  /* used uninitialized */
```

This writes garbage stack data to hardware CRTC registers. The fix should set `*burst` and `*lwm` to safe defaults (e.g., 0) before returning, or the function signature should be changed to return an error that the caller checks.

The simplest fix would be:

```c
if (MClk <= 0 || NVClk <= 0) {
    *burst = 0;
    *lwm = 0;
    return;
}
```

**Minor: the `<= 0` check conflates two cases.** `nouveau_hw_get_clock()` can return:
- `0` — from `nouveau_hw_pllvals_to_clk()` when M1 or M2 is zero
- A negative errno (e.g., `-ENOENT`) — from `nouveau_hw_get_pllvals()` failure

The `<= 0` check correctly catches both, so the logic is sound, but the commit message claims to "fix all this by adding a check" when the real root cause (propagating error codes properly from `nouveau_hw_get_clock`) remains unfixed. This is acceptable for a targeted defensive fix, but worth noting.

**Commit message nit:** The Fixes tag references `6ee738610f41` ("drm/nouveau: Add DRM driver for NVIDIA GPUs"), which is the initial nouveau commit. While technically correct that the bug has existed since then, it would be more useful to reference the commit that introduced this specific code path if one exists, since the Fixes tag drives stable-kernel backport decisions.

**Summary:** The identified bug (division by zero) is real and worth fixing. The patch needs a small revision to initialize the output parameters before the early return to avoid writing uninitialized values to hardware registers.

---
Generated by Claude Code Patch Reviewer