Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Clobbered Hardware Clock Programming Error in `nvkm_cstate_prog` - [High] State Tracking Desync in `nvkm_pstate_prog` - [High] Ignored RAM Clock Programming Error in `nvkm_pstate_prog` -- --- Patch [2]: [PATCH v2 2/4] drm/nouveau/clk: don't use the pstate cursor after the loop --- commit 9e641d48c614d5036ae19e8053df44c4f5085e05 Author: Francesco Magazzu <[email protected]> drm/nouveau/clk: don't use the pstate cursor after the loop nvkm_pstate_prog() walks clk->states looking for an entry and uses the list_for_each_entry cursor after the loop. Although current callers clamp the index to prevent out-of-bounds access, this leaves a trap for future callers. To fix this, the code now tracks whether the entry was found and returns -EINVAL if it was not. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c > index 42f3709e07474..4d546b07f4cbf 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c [ ... ] > @@ -270,13 +270,19 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei) [ ... ] > + if (!found) > + return -EINVAL; > + > nvkm_debug(subdev, "setting performance state %d\n", pstatei); > clk->pstate = pstatei; [Severity: High] This isn't a bug introduced by this patch, but could this prematurely update the software state tracker before hardware programming has completed and been verified? If a hardware failure occurs during nvkm_cstate_prog() or RAM clock programming, the transition fails. Because clk->pstate is updated here before hardware programming finishes, a failure leaves the hardware out of sync with software. Future requests for this state might be incorrectly skipped because the software evaluates pstate != clk->pstate to false, leaving the hardware stuck in an inconsistent state. drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:nvkm_pstate_prog() { ... do { ret = ram->func->calc(ram, khz); if (ret == 0) ret = ram->func->prog(ram); } while (ret > 0); ram->func->tidy(ram); } return nvkm_cstate_prog(clk, pstate, NVKM_CLK_CSTATE_HIGHEST); } [Severity: High] This is also a pre-existing issue, but does nvkm_pstate_prog() silently discard the RAM clock programming error? If ram->func->calc() or ram->func->prog() fails and returns a negative error code (like -EINVAL or a hardware timeout), the error stored in 'ret' is discarded when the loop exits. The function then proceeds to unconditionally return the result of nvkm_cstate_prog(). This decoupling of memory and core clocks could lead to hardware lockups or severe screen corruption due to mismatched frequencies. drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:nvkm_cstate_prog() { ... ret = clk->func->calc(clk, cstate); if (ret == 0) { ret = clk->func->prog(clk); clk->func->tidy(clk); } if (volt) { ret = nvkm_volt_set_id(volt, cstate->voltage, pstate->base.voltage, clk->temp, -1); ... } [Severity: High] This is another pre-existing issue, but is the hardware clock programming error clobbered in nvkm_cstate_prog()? If clk->func->calc() or clk->func->prog() fails and returns a negative error code, the 'ret' variable containing the error is unconditionally overwritten by the return value of nvkm_volt_set_id() (which will be 0 on success). This causes the transition to report success despite a hardware failure, masking the error and corrupting higher-level state tracking. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
