[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c60b572e-12a8-49b1-a75a-b8a7b7ed674b@amd.com>
Date: Mon, 27 May 2024 09:58:11 +0200
From: Christian König <christian.koenig@....com>
To: Zhouyi Zhou <zhouzhouyi@...il.com>, alexander.deucher@....com,
Xinhui.Pan@....com, airlied@...il.com, daniel@...ll.ch,
chris@...isdown.name, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
rcu@...r.kernel.org, lance@...osl.org
Subject: Re: [PATCH] drm/radeon/r100: enhance error handling in
r100_cp_init_microcode
Am 27.05.24 um 03:20 schrieb Zhouyi Zhou:
> In r100_cp_init_microcode, if rdev->family don't match any of
> if statement, fw_name will be NULL, which will cause
> gcc (11.4.0 powerpc64le-linux-gnu) complain:
>
> In function ‘r100_cp_init_microcode’,
> inlined from ‘r100_cp_init’ at drivers/gpu/drm/radeon/r100.c:1136:7:
> ./include/linux/printk.h:457:44: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> 457 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
>
> Above warning is emitted during the rcutorture test in
> in PPC VM of Opensource Lab of Oregon State Univerisity.
>
> Enhance error handling in r100_cp_init_microcode, let r100_cp_init_microcode
> return with -EINVAL when none of chip families is matched.
>
> Signed-off-by: Zhouyi Zhou <zhouzhouyi@...il.com>
> ---
> drivers/gpu/drm/radeon/r100.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> index 0b1e19345f43..4f8a1bdd9365 100644
> --- a/drivers/gpu/drm/radeon/r100.c
> +++ b/drivers/gpu/drm/radeon/r100.c
> @@ -1055,6 +1055,11 @@ static int r100_cp_init_microcode(struct radeon_device *rdev)
> (rdev->family == CHIP_RV570)) {
> DRM_INFO("Loading R500 Microcode\n");
> fw_name = FIRMWARE_R520;
> + } else {
> + pr_err("radeon_cp: Failed to load firmware \"%d\"\n",
> + rdev->family);
> + err = -EINVAL;
> + goto out;
> }
>
> err = request_firmware(&rdev->me_fw, fw_name, rdev->dev);
> @@ -1067,6 +1072,8 @@ static int r100_cp_init_microcode(struct radeon_device *rdev)
> release_firmware(rdev->me_fw);
> rdev->me_fw = NULL;
> }
> +
> +out:
That looks superfluous, just return -EINVAL directly in the else case above.
Apart from that this is for ~15year old hardware. I'm a bit reluctant
adding code for something that old even when this change here looks
harmless.
Is there a plan to complain about that in an automated checker? If yes
then the change is probably justified, if no then I would rather not do it.
Regards,
Christian.
> return err;
> }
>
Powered by blists - more mailing lists