Re: [PATCH 01/18] drm/amdgpu: enhance amdgpu_ucode_request() function flexibility

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 31.05.24 um 08:52 schrieb Yang Wang:
> Adding formatting string feature to improve function flexibility.
>
> Signed-off-by: Yang Wang <kevinyang.wang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 30 +++++++++++++++++------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h |  3 ++-
>   2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index a9de78bb96e2..a452d9b6afdb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -28,6 +28,8 @@
>   #include "amdgpu.h"
>   #include "amdgpu_ucode.h"
>   
> +#define AMDGPU_MAX_FW_NAME_LEN		(128)

Please drop that define and use NAME_MAX instead.

> +
>   static void amdgpu_ucode_print_common_hdr(const struct common_firmware_header *hdr)
>   {
>   	DRM_DEBUG("size_bytes: %u\n", le32_to_cpu(hdr->size_bytes));
> @@ -1432,28 +1434,40 @@ void amdgpu_ucode_ip_version_decode(struct amdgpu_device *adev, int block_type,
>    *
>    * @adev: amdgpu device
>    * @fw: pointer to load firmware to
> - * @fw_name: firmware to load
> + * @fmt: firmware name format string
> + * @...: variable arguments
>    *
>    * This is a helper that will use request_firmware and amdgpu_ucode_validate
>    * to load and run basic validation on firmware. If the load fails, remap
>    * the error code to -ENODEV, so that early_init functions will fail to load.
>    */
>   int amdgpu_ucode_request(struct amdgpu_device *adev, const struct firmware **fw,
> -			 const char *fw_name)
> +			 const char *fmt, ...)
>   {
> -	int err = request_firmware(fw, fw_name, adev->dev);
> +	char fname[AMDGPU_MAX_FW_NAME_LEN];
> +	va_list ap;
> +	int r;
> +
> +	va_start(ap, fmt);
> +	r = vsnprintf(fname, sizeof(fname), fmt, ap);
> +	va_end(ap);
> +	if (r == sizeof(fname)) {
> +		dev_warn(adev->dev, "amdgpu firmware name buffer overflow\n");
> +		return -EOVERFLOW;
> +	}
>   
> -	if (err)
> +	r = request_firmware(fw, fname, adev->dev);
> +	if (r)
>   		return -ENODEV;
>   
> -	err = amdgpu_ucode_validate(*fw);
> -	if (err) {
> -		dev_dbg(adev->dev, "\"%s\" failed to validate\n", fw_name);
> +	r = amdgpu_ucode_validate(*fw);
> +	if (r) {
> +		dev_dbg(adev->dev, "\"%s\" failed to validate\n", fname);
>   		release_firmware(*fw);
>   		*fw = NULL;
>   	}
>   
> -	return err;
> +	return r;
>   }
>   
>   /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> index db745ab7b0c8..5bc37acd3981 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> @@ -594,8 +594,9 @@ void amdgpu_ucode_print_rlc_hdr(const struct common_firmware_header *hdr);
>   void amdgpu_ucode_print_sdma_hdr(const struct common_firmware_header *hdr);
>   void amdgpu_ucode_print_psp_hdr(const struct common_firmware_header *hdr);
>   void amdgpu_ucode_print_gpu_info_hdr(const struct common_firmware_header *hdr);
> +__printf(3, 4)
>   int amdgpu_ucode_request(struct amdgpu_device *adev, const struct firmware **fw,
> -			 const char *fw_name);
> +			 const char *fmt, ...);

That should probably have __printf() annotation so that the compiler can 
check the format and arguments.

See here 
https://elixir.free-electrons.com/linux/v6.10-rc2/source/include/linux/compiler_attributes.h#L171

Regards,
Christian.

>   void amdgpu_ucode_release(const struct firmware **fw);
>   bool amdgpu_ucode_hdr_version(union amdgpu_firmware_header *hdr,
>   				uint16_t hdr_major, uint16_t hdr_minor);