From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/mgag200: validate pixel clock against PLL range in mode_valid Date: Thu, 04 Jun 2026 12:48:08 +1000 Message-ID: In-Reply-To: <1780402238.22b24b6f590c@berkoc.com> References: <1780402238.22b24b6f590c@berkoc.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: drm/mgag200: validate pixel clock against PLL range in mode_valid Author: Berkant Koc 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 =3D n =3D p = =3D s =3D 0`. The subsequent `atomic_update` then computes `pixpllc->m - 1`= (unsigned underflow), programming garbage into the PLL registers. The approach =E2=80=94 filtering at `mode_valid` time via an optional per-m= odel callback =E2=80=94 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` coun= terparts in the current tree. The VCO constants (`vcomax`, `vcomin`, `pllre= ffreq`), loop ranges, computation formulas, and VCO guard conditions all ma= tch exactly across all six models (G200EH, G200EH3, G200ER, G200EV, G200EW3= , G200WB). The bug description in the commit message is accurate =E2=80=94 = each `atomic_check` initializes `m =3D n =3D p =3D s =3D 0` and returns 0 u= nconditionally. **Main concerns:** 1. **Significant code duplication** =E2=80=94 Each `mode_valid` duplicates = the entire search loop from its `atomic_check` counterpart (just without sa= ving `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 sha= red helper that `mode_valid` calls (checking "was a valid candidate found?"= ) and `atomic_check` also calls (saving the best candidate). This would eli= minate ~200 lines of near-duplicate code. 2. **Missing `Fixes:` tag** =E2=80=94 The commit message describes a genuin= e bug with real symptoms (corrupted output clock, unsigned underflow). A `F= ixes:` 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** =E2=80=94 The commit message honestly notes "Test= ed with static analysis ... runtime confirmation on physical Matrox hardwar= e welcome." For a display driver change, runtime testing on at least one af= fected 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 factor= ed or duplicated approach is a judgment call. --- --- Generated by Claude Code Patch Reviewer