Re: [PATCH v6] drm/amd: Select the firmware signing scheme the PSP reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/amd: commit message and comment mismatch on
  AMDGPU_FW_SIGN_UNKNOWN
--

commit 1e462a80e1c59a04ecf750861d9cb9ad5ced28ac
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/amd: Select the firmware signing scheme the PSP reports

This commit reads C2PMSG_63 on MP0 15.0.0 and 15.0.9 to determine the
PSP firmware signing scheme and preproduction status, and appends the
corresponding "_l" or "_p" suffix when requesting firmware.

>     Add amdgpu_psp_init_fw_variant() which reads those bits through amdgpu's
>     own MMIO, and keep the result in adev->firmware.sign_type next to
>     load_type, where preproduction is a type of its own. It is initialized
>     once, after IP discovery has populated the register bases and only when
>     firmware is loaded through the front door (the PSP). Only MP0 15.0.0
>     reports it. For all other ASICs, SR-IOV VFs and an inaccessible register
>     (all ones) the defaults are kept: AMDGPU_FW_SIGN_DEFAULT. A preproduction
>     part that also reports the alternative scheme is not expected, so warn
>     about it and use the default firmware.

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index c0ed3e24b6840..a995d948284cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -403,6 +403,26 @@ static uint64_t psp_get_runtime_db_header_pos(struct amdgpu_device *adev)
>  	return pos;
>  }
>  
> +/*
> + * Which firmware the platform needs, as reported by the PSP in C2PMSG_63: the
> + * signing scheme and whether this is a preproduction part. The defaults are kept if
> + * the ASIC doesn't report it or it can't be read.
> + */
> +void amdgpu_psp_init_fw_variant(struct amdgpu_device *adev)
> +{
> +	if (amdgpu_sriov_vf(adev))
> +		return;
> +
> +	switch (amdgpu_ip_version(adev, MP0_HWIP, 0)) {
> +	case IP_VERSION(15, 0, 0):
> +	case IP_VERSION(15, 0, 9):
> +		psp_v15_0_0_get_fw_variant(adev);
> +		break;
> +	default:
> +		break;
> +	}
> +}

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index d2ce526dd95cc..1632eda91acd9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -1527,6 +1527,37 @@ void amdgpu_ucode_ip_version_decode(struct amdgpu_device *adev, int block_type,
>  	snprintf(ucode_prefix, len, "%s_%d_%d_%d", ip_name, maj, min, rev);
>  }
>  
> +/* Add the suffix of the firmware variant before the extension: foo.bin -> foo_l.bin */
> +static int amdgpu_ucode_set_suffix(struct amdgpu_device *adev, char *fname)
> +{
> +	const char *suffix;
> +	size_t base;
> +
> +	switch (adev->firmware.sign_type) {
> +	case AMDGPU_FW_SIGN_PREPROD:
> +		suffix = "_p.bin";
> +		break;
> +	case AMDGPU_FW_SIGN_ALT:
> +		suffix = "_l.bin";
> +		break;
> +	case AMDGPU_FW_SIGN_UNKNOWN:
> +		drm_warn(adev_to_drm(adev), "Unknown firmware signing\n");
> +		return -EINVAL;
> +	default:
> +		/* Default variant: the name is unchanged */
> +		return 0;
> +	}

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> index 7c809692807fc..15e789718dede 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> @@ -571,6 +571,14 @@ enum amdgpu_firmware_load_type {
>  	AMDGPU_FW_LOAD_RLC_BACKDOOR_AUTO,
>  };
>  
> +enum amdgpu_firmware_sign_type {
> +	AMDGPU_FW_SIGN_DEFAULT = 0,
> +	AMDGPU_FW_SIGN_ALT,
> +	AMDGPU_FW_SIGN_PREPROD,
> +	/* Unexpected combination of signing bits, default firmware may not work */
> +	AMDGPU_FW_SIGN_UNKNOWN,
> +};

[Severity: Low]
This isn't a bug, but the commit message and the comment above
AMDGPU_FW_SIGN_UNKNOWN do not match the code.

When both alt and preprod bits are set, psp_v15_0_0_get_fw_variant() sets
AMDGPU_FW_SIGN_UNKNOWN and returns early without logging C2PMSG_63, and
amdgpu_ucode_set_suffix() returns -EINVAL rather than falling back to the
default firmware.  Also, the commit message says only MP0 15.0.0 reports
this, whereas amdgpu_psp_init_fw_variant() queries both 15.0.0 and
15.0.9.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114160925.390350-1-superm1@kernel.org?part=1