Re: [PATCH 2/5] drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `r535_gsp_get_static_memsys_info` function allocates an RPC command payload but never actually executes the command before reading its fields, resulting in reading uninitialized memory.
- [Medium] The memsys static info is never fetched when running on r535 firmware because the call to `r535_gsp_get_static_memsys_info` is omitted from `r535_gsp_get_static_info`.
--

commit 80c8c37934b6a701a8974b57594b260a8c0690c0
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

This patch adds support for retrieving a GSP structure describing memory
management system characteristics during driver load. It fetches the
configuration for MEMSYS_GET_STATIC_CONFIG and saves it for later use.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index 09b17b85dc1ac..c57a3250d6b96 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
[ ... ]
> @@ -1808,6 +1808,23 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
>  	return 0;
>  }
>  
> +int
> +r535_gsp_get_static_memsys_info(struct nvkm_gsp *gsp)
> +{
> +	NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS *ctrl;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.subdevice,
> +				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG,
> +				    sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	gsp->memsys.use_raw_mode_comptagline_alloc = ctrl->bUseRawModeComptaglineAllocation;

[Severity: High]
Does this actually execute the RPC command to fetch the data?

It looks like nvkm_gsp_rm_ctrl_get() only allocates the payload buffer in
memory, but without a subsequent call to nvkm_gsp_rm_ctrl_push() (or by using
nvkm_gsp_rm_ctrl_rd() instead), the command is never pushed to the firmware.

As a result, this might be reading uninitialized memory instead of the
firmware's response. This could silently break the workaround introduced in
the subsequent commit "drm/nouveau/gsp/r570: Add comp mode workaround from
issue #3172217", as use_raw_mode_comptagline_alloc will contain garbage or
zero, preventing the workaround from being applied properly.

> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> index b45781cd0dfdc..5a51fe7e5ab98 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> @@ -137,6 +137,14 @@ r570_gsp_get_static_info(struct nvkm_gsp *gsp)
>  		}
>  	}
>  
> +	ret = r535_gsp_get_static_memsys_info(gsp);

[Severity: Medium]
Should r535_gsp_get_static_info() also be updated to call
r535_gsp_get_static_memsys_info(gsp)?

Currently, this is only added to r570_gsp_get_static_info(), which means the
memsys static info is never fetched when running on r535 firmware, and
gsp->memsys.use_raw_mode_comptagline_alloc will remain uninitialized for r535.

While the follow-up commit "drm/nouveau/gsp/r570: Add comp mode workaround
from issue #3172217" currently only uses this flag in r570-specific suspend
paths, the commit message implies this configuration should be fetched on
r535 as well. Any future shared code relying on this flag on r535 could
receive uninitialized data.

> +	if (ret) {
> +		nvkm_error(&gsp->subdev, "Retrieving static memsys info failed\n");
> +		return ret;
> +	}
> +	nvkm_debug(&gsp->subdev, "memsys: Use raw mode for comptag allocations? %s\n",
> +		   str_yes_no(gsp->memsys.use_raw_mode_comptagline_alloc));
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=2