On Wed, 2023-03-01 at 18:25 +0100, Jakob Koschel wrote: > If potentially no valid element is found, 'pstate' would contain an > invalid pointer past the iterator loop. To ensure 'pstate' is always > valid, we only set it if the correct element was found. That allows > adding a BUG_ON in case the code works incorrectly, exposing currently > undetectable potential bugs. > > Additionally, Linus proposed to avoid any use of the list iterator > variable after the loop, in the attempt to move the list iterator > variable declaration into the marcro to avoid any potential misuse after > the loop [1]. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@xxxxxxxxxxxxxx/ [1] > Signed-off-by: Jakob Koschel <jkl820.git@xxxxxxxxx> > --- > drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > index ce774579c89d..7c9dd91e98ee 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > @@ -72,7 +72,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) > } *args = data; > struct nvkm_clk *clk = ctrl->device->clk; > const struct nvkm_domain *domain; > - struct nvkm_pstate *pstate; > + struct nvkm_pstate *pstate = NULL, *iter; > struct nvkm_cstate *cstate; > int i = 0, j = -1; > u32 lo, hi; > @@ -103,11 +103,14 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) > return -EINVAL; > > if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) { > - list_for_each_entry(pstate, &clk->states, head) { > - if (i++ == args->v0.state) > + list_for_each_entry(iter, &clk->states, head) { > + if (i++ == args->v0.state) { > + pstate = iter; > break; > + } > } > > + BUG_ON(!pstate); Let's replace this with if (WARN_ON_ONCE(!pstate)) return -EINVAL; > lo = pstate->base.domain[domain->name]; > hi = lo; > list_for_each_entry(cstate, &pstate->list, head) { > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
