Re: [PATCH v2 4/4] drm/nouveau/clk: don't clobber reclock status when restoring volt/fan

[lyude]

Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

On Sun, 2026-07-12 at 14:36 +0200, Francesco Magazzu wrote:
> nvkm_cstate_prog() reuses 'ret' for the voltage and fan-speed restore
> calls it makes after reprogramming the clocks.  Those calls almost
> always
> succeed, so the status of the reclock itself is overwritten and the
> function reports success even when clk->func->calc() or clk->func-
> >prog()
> failed.  The converse is also true: a successful reclock is reported
> as an
> error if the final restore call fails, even though that failure is
> only
> logged and otherwise ignored.
> 
> The only consumer of the return value is the error message in
> nvkm_pstate_work(), so in practice a failing reclock is simply never
> reported.  Nothing else changes, but a function that returns success
> on
> failure is a trap for the next caller.
> 
> Keep the calc/prog status in 'ret' and use a separate local for the
> restore calls.
> 
> Fixes: 3eca809b3c05 ("drm/nouveau/clk: cosmetic changes")
> Signed-off-by: Francesco Magazzu <postadelmaga@xxxxxxxxx>
> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> index 4d546b07f..05336fc7d 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -199,16 +199,18 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct
> nvkm_pstate *pstate, int cstatei)
>  	}
>  
>  	if (volt) {
> -		ret = nvkm_volt_set_id(volt, cstate->voltage,
> -				       pstate->base.voltage, clk-
> >temp, -1);
> -		if (ret && ret != -ENODEV)
> -			nvkm_error(subdev, "failed to lower voltage:
> %d\n", ret);
> +		int err = nvkm_volt_set_id(volt, cstate->voltage,
> +					   pstate->base.voltage,
> clk->temp, -1);
> +
> +		if (err && err != -ENODEV)
> +			nvkm_error(subdev, "failed to lower voltage:
> %d\n", err);
>  	}
>  
>  	if (therm) {
> -		ret = nvkm_therm_cstate(therm, pstate->fanspeed, -
> 1);
> -		if (ret && ret != -ENODEV)
> -			nvkm_error(subdev, "failed to lower fan
> speed: %d\n", ret);
> +		int err = nvkm_therm_cstate(therm, pstate->fanspeed,
> -1);
> +
> +		if (err && err != -ENODEV)
> +			nvkm_error(subdev, "failed to lower fan
> speed: %d\n", err);
>  	}
>  
>  	return ret;