[PATCH v2] drm/mgag200: validate pixel clock against PLL range in mode_valid

[PATCH v2] drm/mgag200: validate pixel clock against PLL range in mode_valid

[Berkant Koc]

The per-model pixpllc_atomic_check helpers for G200WB, G200EH, G200ER,
G200EV, G200EH3 and G200EW3 initialise m, n, p and s to zero and only
update them when the inner search loop finds a candidate whose delta
to the requested clock improves on the previous best. They return 0
unconditionally afterwards. If the requested pixel clock falls outside
the model's VCO window so that the outer guard skips every iteration,
no candidate is recorded and atomic_update later programs pixpllc->m - 1
(unsigned underflow) into PIXPLLCM/N/P. The result is a corrupted
output clock on those models.

Following Thomas Zimmermann's suggestion, reject such modes earlier in
the pipeline rather than inside atomic_check. Add an optional
pixpllc_mode_valid callback to struct mgag200_device_funcs and dispatch
it from the shared mgag200_crtc_helper_mode_valid. Each affected helper
provides an implementation that walks the same divider space as its
atomic_check counterpart and returns MODE_CLOCK_RANGE when no candidate
falls within the same 0.5% (5/1000) tolerance that g200se has always
enforced. Models without the callback (G200, G200SE, G200EH5) keep
their current behaviour.

Filtering at mode_valid time means the userspace mode list never
contains modes the PLL cannot synthesise, so atomic_check no longer has
to defend against an empty search loop and atomic_update is never
reached with zeroed pixpllc values.

Tested with static analysis against the linux-7.1-rc4 tree; runtime
confirmation on physical Matrox hardware welcome. checkpatch.pl --strict
clean.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Berkant Koc <me@berkoc.com>
Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
---
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  8 ++++
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  | 41 +++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 38 +++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_g200er.c  | 50 +++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  | 42 +++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 45 ++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  | 41 +++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c    | 13 ++++++
 8 files changed, 278 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index a875c4bf8c..3eae8756cc 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -248,6 +248,14 @@ struct mgag200_device_info {
 	}
 
 struct mgag200_device_funcs {
+	/*
+	 * Optional. Validate that @mode's pixel clock falls within the
+	 * model's PIXPLLC window. Called from mgag200_crtc_helper_mode_valid
+	 * so that out-of-range modes are filtered before atomic_check.
+	 */
+	enum drm_mode_status (*pixpllc_mode_valid)(struct drm_crtc *crtc,
+						   const struct drm_display_mode *mode);
+
 	/*
 	 * Validate that the given state can be programmed into PIXPLLC. On
 	 * success, the calculated parameters should be stored in the CRTC's
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index d2aa931f57..189bb1671e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -40,6 +40,46 @@ void mgag200_g200eh_init_registers(struct mga_device *mdev)
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200eh_pixpllc_mode_valid(struct drm_crtc *crtc,
+							      const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 800000;
+	static const unsigned int vcomin = 400000;
+	static const unsigned int pllreffreq = 33333;
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testp, testm, testn;
+	unsigned int computed;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testp = 16; testp > 0; testp >>= 1) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
+
+		for (testm = 1; testm < 33; testm++) {
+			for (testn = 17; testn < 257; testn++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta)
+					delta = tmpdelta;
+			}
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200eh_pixpllc_atomic_check(struct drm_crtc *crtc,
 					       struct drm_atomic_state *new_state)
 {
@@ -230,6 +270,7 @@ static const struct mgag200_device_info mgag200_g200eh_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 37500, false, 1, 0, false);
 
 static const struct mgag200_device_funcs mgag200_g200eh_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200eh_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200eh_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200eh_pixpllc_atomic_update,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 7bea7a728f..621e4efb47 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -15,6 +15,43 @@
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200eh3_pixpllc_mode_valid(struct drm_crtc *crtc,
+							       const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 3000000;
+	static const unsigned int vcomin = 1500000;
+	static const unsigned int pllreffreq = 25000;
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testm, testn;
+	unsigned int computed;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testm = 150; testm >= 6; testm--) {
+		if (clock * testm > vcomax)
+			continue;
+		if (clock * testm < vcomin)
+			continue;
+		for (testn = 120; testn >= 60; testn--) {
+			computed = (pllreffreq * testn) / testm;
+			if (computed > clock)
+				tmpdelta = computed - clock;
+			else
+				tmpdelta = clock - computed;
+			if (tmpdelta < delta)
+				delta = tmpdelta;
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200eh3_pixpllc_atomic_check(struct drm_crtc *crtc,
 						struct drm_atomic_state *new_state)
 {
@@ -134,6 +171,7 @@ static const struct mgag200_device_info mgag200_g200eh3_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, false, 1, 0, false);
 
 static const struct mgag200_device_funcs mgag200_g200eh3_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200eh3_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200eh3_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200eh_pixpllc_atomic_update, // same as G200EH
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index 8fa8fe943a..d030094ae4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -57,6 +57,55 @@ static void mgag200_g200er_reset_tagfifo(struct mga_device *mdev)
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200er_pixpllc_mode_valid(struct drm_crtc *crtc,
+							      const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 1488000;
+	static const unsigned int vcomin = 1056000;
+	static const unsigned int pllreffreq = 48000;
+	static const unsigned int m_div_val[] = { 1, 2, 4, 8 };
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	int testr, testn, testm, testo;
+	unsigned int computed, vco;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testr = 0; testr < 4; testr++) {
+		if (delta == 0)
+			break;
+		for (testn = 5; testn < 129; testn++) {
+			if (delta == 0)
+				break;
+			for (testm = 3; testm >= 0; testm--) {
+				if (delta == 0)
+					break;
+				for (testo = 5; testo < 33; testo++) {
+					vco = pllreffreq * (testn + 1) / (testr + 1);
+					if (vco < vcomin)
+						continue;
+					if (vco > vcomax)
+						continue;
+					computed = vco / (m_div_val[testm] * (testo + 1));
+					if (computed > clock)
+						tmpdelta = computed - clock;
+					else
+						tmpdelta = clock - computed;
+					if (tmpdelta < delta)
+						delta = tmpdelta;
+				}
+			}
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200er_pixpllc_atomic_check(struct drm_crtc *crtc,
 					       struct drm_atomic_state *new_state)
 {
@@ -267,6 +316,7 @@ static const struct mgag200_device_info mgag200_g200er_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 55000, false, 1, 0, false);
 
 static const struct mgag200_device_funcs mgag200_g200er_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200er_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200er_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200er_pixpllc_atomic_update,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 3fadbeb10a..8286e4d6ad 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -46,6 +46,47 @@ static void mgag200_g200ev_set_hiprilvl(struct mga_device *mdev)
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200ev_pixpllc_mode_valid(struct drm_crtc *crtc,
+							      const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 550000;
+	static const unsigned int vcomin = 150000;
+	static const unsigned int pllreffreq = 50000;
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testp, testm, testn;
+	unsigned int computed;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testp = 16; testp > 0; testp--) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
+
+		for (testn = 1; testn < 257; testn++) {
+			for (testm = 1; testm < 17; testm++) {
+				computed = (pllreffreq * testn) /
+					(testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta)
+					delta = tmpdelta;
+			}
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200ev_pixpllc_atomic_check(struct drm_crtc *crtc,
 					       struct drm_atomic_state *new_state)
 {
@@ -268,6 +309,7 @@ static const struct mgag200_device_info mgag200_g200ev_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 32700, false, 0, 1, false);
 
 static const struct mgag200_device_funcs mgag200_g200ev_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200ev_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200ev_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200ev_pixpllc_atomic_update,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index e387a455ea..8db95f4d13 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -22,6 +22,50 @@ static void mgag200_g200ew3_init_registers(struct mga_device *mdev)
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200ew3_pixpllc_mode_valid(struct drm_crtc *crtc,
+							       const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 800000;
+	static const unsigned int vcomin = 400000;
+	static const unsigned int pllreffreq = 25000;
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testp, testm, testn, testp2;
+	unsigned int computed;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testp = 1; testp < 8; testp++) {
+		for (testp2 = 1; testp2 < 8; testp2++) {
+			if (testp < testp2)
+				continue;
+			if ((clock * testp * testp2) > vcomax)
+				continue;
+			if ((clock * testp * testp2) < vcomin)
+				continue;
+			for (testm = 1; testm < 26; testm++) {
+				for (testn = 32; testn < 2048; testn++) {
+					computed = (pllreffreq * testn) /
+						(testm * testp * testp2);
+					if (computed > clock)
+						tmpdelta = computed - clock;
+					else
+						tmpdelta = clock - computed;
+					if (tmpdelta < delta)
+						delta = tmpdelta;
+				}
+			}
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200ew3_pixpllc_atomic_check(struct drm_crtc *crtc,
 						struct drm_atomic_state *new_state)
 {
@@ -143,6 +187,7 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false);
 
 static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200ew3_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same as G200WB
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index d847fa8ded..63f9909a99 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -38,6 +38,46 @@ void mgag200_g200wb_init_registers(struct mga_device *mdev)
  * PIXPLLC
  */
 
+static enum drm_mode_status mgag200_g200wb_pixpllc_mode_valid(struct drm_crtc *crtc,
+							      const struct drm_display_mode *mode)
+{
+	static const unsigned int vcomax = 550000;
+	static const unsigned int vcomin = 150000;
+	static const unsigned int pllreffreq = 48000;
+
+	long clock = mode->clock;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testp, testm, testn;
+	unsigned int computed;
+
+	if (clock <= 0)
+		return MODE_CLOCK_LOW;
+
+	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
+
+	for (testp = 1; testp < 9; testp++) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
+
+		for (testm = 1; testm < 17; testm++) {
+			for (testn = 1; testn < 151; testn++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta)
+					delta = tmpdelta;
+			}
+		}
+	}
+
+	return (delta > permitteddelta) ? MODE_CLOCK_RANGE : MODE_OK;
+}
+
 static int mgag200_g200wb_pixpllc_atomic_check(struct drm_crtc *crtc,
 					       struct drm_atomic_state *new_state)
 {
@@ -277,6 +317,7 @@ static const struct mgag200_device_info mgag200_g200wb_device_info =
 	MGAG200_DEVICE_INFO_INIT(1280, 1024, 31877, true, 0, 1, false);
 
 static const struct mgag200_device_funcs mgag200_g200wb_device_funcs = {
+	.pixpllc_mode_valid = mgag200_g200wb_pixpllc_mode_valid,
 	.pixpllc_atomic_check = mgag200_g200wb_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 8894a063b1..ed82171a84 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -583,6 +583,8 @@ enum drm_mode_status mgag200_crtc_helper_mode_valid(struct drm_crtc *crtc,
 {
 	struct mga_device *mdev = to_mga_device(crtc->dev);
 	const struct mgag200_device_info *info = mdev->info;
+	const struct mgag200_device_funcs *funcs = mdev->funcs;
+	enum drm_mode_status status;
 
 	/*
 	 * Some devices have additional limits on the size of the
@@ -605,6 +607,17 @@ enum drm_mode_status mgag200_crtc_helper_mode_valid(struct drm_crtc *crtc,
 		return MODE_BAD;
 	}
 
+	/*
+	 * Reject modes whose pixel clock falls outside the per-model PIXPLLC
+	 * window. Without this filter, pixpllc_atomic_check can return success
+	 * with zeroed PLL parameters and atomic_update would underflow.
+	 */
+	if (funcs->pixpllc_mode_valid) {
+		status = funcs->pixpllc_mode_valid(crtc, mode);
+		if (status != MODE_OK)
+			return status;
+	}
+
 	return MODE_OK;
 }
 
-- 
2.47.3

Claude review: drm/mgag200: validate pixel clock against PLL range in mode_valid

[Claude Code Review Bot]

Overall Series Review

Subject: drm/mgag200: validate pixel clock against PLL range in mode_valid
Author: Berkant Koc <me@berkoc.com>
Patches: 1
Reviewed: 2026-06-04T12:48:08.407892

---

This is a single well-structured patch (v2) that addresses a real bug: when a requested pixel clock falls outside a model's VCO window, the PLL search loop in `pixpllc_atomic_check` finds no candidate, leaving `m = n = p = s = 0`. The subsequent `atomic_update` then computes `pixpllc->m - 1` (unsigned underflow), programming garbage into the PLL registers.

The approach — filtering at `mode_valid` time via an optional per-model callback — is sound and follows Thomas Zimmermann's suggestion. It catches bad modes before they reach `atomic_check`, so userspace never sees modes the PLL cannot synthesize.

I verified all six `mode_valid` functions against their `atomic_check` counterparts in the current tree. The VCO constants (`vcomax`, `vcomin`, `pllreffreq`), loop ranges, computation formulas, and VCO guard conditions all match exactly across all six models (G200EH, G200EH3, G200ER, G200EV, G200EW3, G200WB). The bug description in the commit message is accurate — each `atomic_check` initializes `m = n = p = s = 0` and returns 0 unconditionally.

**Main concerns:**

1. **Significant code duplication** — Each `mode_valid` duplicates the entire search loop from its `atomic_check` counterpart (just without saving `m/n/p/s`). This creates a maintenance risk: if VCO constants or loop ranges are updated in `atomic_check`, the `mode_valid` copy must be updated in lockstep. Consider whether the search loop could be factored into a shared helper that `mode_valid` calls (checking "was a valid candidate found?") and `atomic_check` also calls (saving the best candidate). This would eliminate ~200 lines of near-duplicate code.

2. **Missing `Fixes:` tag** — The commit message describes a genuine bug with real symptoms (corrupted output clock, unsigned underflow). A `Fixes:` tag referencing the commit that introduced the unconditional `return 0` in these `atomic_check` functions would help stable kernel maintainers identify this for backporting. A `Cc: stable@vger.kernel.org` would also be appropriate.

3. **No runtime testing** — The commit message honestly notes "Tested with static analysis ... runtime confirmation on physical Matrox hardware welcome." For a display driver change, runtime testing on at least one affected model would significantly increase confidence.

**The patch is correct as-is.** The duplication concern is about long-term maintainability, not correctness. Whether the maintainer prefers the factored or duplicated approach is a judgment call.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/mgag200: validate pixel clock against PLL range in mode_valid

[Claude Code Review Bot]

Patch Review

**Correctness of the framework plumbing (mgag200_drv.h + mgag200_mode.c):**

The new optional callback in `mgag200_device_funcs` is well-placed — it's added before `pixpllc_atomic_check`, which is the logical ordering. The dispatch in `mgag200_crtc_helper_mode_valid` correctly checks for NULL before calling:

```c
if (funcs->pixpllc_mode_valid) {
    status = funcs->pixpllc_mode_valid(crtc, mode);
    if (status != MODE_OK)
        return status;
}
```

This is clean and correctly returns early on failure.

**G200EH mode_valid — verified correct:**

Constants and loop ranges match `mgag200_g200eh_pixpllc_atomic_check` exactly:
- `vcomax=800000, vcomin=400000, pllreffreq=33333`
- `testp: 16→1 (>>= 1)`, `testm: 1..<33`, `testn: 17..<257`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**G200EH3 mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=3000000, vcomin=1500000, pllreffreq=25000`
- `testm: 150→6 (--)`, `testn: 120→60 (--)`
- Formula: `(pllreffreq * testn) / testm` ✓
- Correctly omits `testp` from computation (it's a constant 0 in atomic_check, only used as `p = testp + 1` when saving)

Minor: The `atomic_check` has `if (delta == 0) break;` early exits inside both loops; the `mode_valid` omits them. This is functionally correct (just slightly less efficient at mode enumeration time) but is an unnecessary inconsistency with the G200ER `mode_valid`, which does include the early breaks.

**G200ER mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=1488000, vcomin=1056000, pllreffreq=48000`
- `m_div_val[] = { 1, 2, 4, 8 }` ✓
- `testr: 0..<4`, `testn: 5..<129`, `testm: 3→0 (--)`, `testo: 5..<33`
- Formula: `vco = pllreffreq * (testn + 1) / (testr + 1)`, `computed = vco / (m_div_val[testm] * (testo + 1))` ✓
- Correctly includes the `delta == 0` early break optimization

**G200EV mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=550000, vcomin=150000, pllreffreq=50000`
- `testp: 16→1 (--)`, `testn: 1..<257`, `testm: 1..<17`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**G200EW3 mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=800000, vcomin=400000, pllreffreq=25000`
- `testp: 1..<8`, `testp2: 1..<8`, `testm: 1..<26`, `testn: 32..<2048`
- Guard: `testp < testp2` → continue ✓
- VCO check: `clock * testp * testp2` against bounds ✓
- Formula: `(pllreffreq * testn) / (testm * testp * testp2)` ✓

**G200WB mode_valid — verified correct:**

Constants and ranges match:
- `vcomax=550000, vcomin=150000, pllreffreq=48000`
- `testp: 1..<9`, `testm: 1..<17`, `testn: 1..<151`
- Formula: `(pllreffreq * testn) / (testm * testp)` ✓

**Tolerance choice:**

All six `mode_valid` functions use `permitteddelta = clock * 5 / 1000` (0.5%). The commit message states this matches G200SE's existing enforcement. This is a reasonable tolerance — if the best achievable clock is off by more than 0.5%, the mode genuinely cannot be synthesized acceptably. This is more conservative than what the `atomic_check` does today (accepts any delta, however large), which is the right direction.

**Summary of suggestions:**

1. Add a `Fixes:` tag and `Cc: stable@vger.kernel.org`
2. Consider factoring the search loop into a shared helper to eliminate ~200 lines of duplicated logic and prevent future drift between `mode_valid` and `atomic_check`
3. Minor style nit: the `delta == 0` early-break optimization is present in G200ER's `mode_valid` but absent from G200EH3's, despite both atomic_check counterparts having it — should be consistent one way or the other

---
Generated by Claude Code Patch Reviewer