Received-SPF: pass (google.com: domain of linux-kernel+bounces-190181-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <c60b572e-12a8-49b1-a75a-b8a7b7ed674b@amd.com>
Date: Mon, 27 May 2024 09:58:11 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/radeon/r100: enhance error handling in
 r100_cp_init_microcode
To: Zhouyi Zhou <zhouzhouyi@gmail.com>, alexander.deucher@amd.com,
 Xinhui.Pan@amd.com, airlied@gmail.com, daniel@ffwll.ch,
 chris@chrisdown.name, amd-gfx@lists.freedesktop.org,
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
 rcu@vger.kernel.org, lance@osuosl.org
References: <20240527012018.351223-1-zhouzhouyi@gmail.com>
Content-Language: en-US
From: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <20240527012018.351223-1-zhouzhouyi@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bWw4a0d2OU56dytFbm9ZSU9IZUNFRGlxZjFRamVNdUV1SjF1LytEVkQrUVhW?=
 =?utf-8?B?dXl1dWo4OWJkWHlsOUVTRlNSbmV4UjU4UUY3QWxJcU5sNGhJZlk5SXZYYmJC?=
 =?utf-8?B?SW5qYmZnOUh0TjZVZzNnc25TZ25BRWdXbjhOTkVXTFczK2dRZE1TSFd5VUhE?=
 =?utf-8?B?c3UyQjM2Z0Y0cUZ1MjRYcEFQeUZKejFKbFhhNVdCdUFUbWl4Qy8xNFV4K3BJ?=
 =?utf-8?B?cThXL3ZEY0pEeDJ4NVBXNkFpaVRjUlZjaUNqQVVxQ1kvNFJlakIxRlZnaXdF?=
 =?utf-8?B?M0JFTm9vS2IrOEJVTFZDZUJoQWExQXQ4Z3RTbFlrNW5mT0JNWTg1SVlVSW9s?=
 =?utf-8?B?bEtxOTNGMnlZWHRsY1dlazhTL0dYZnlxVXhBWGtmRWRvT0ZTU0ZiR2h1QlVv?=
 =?utf-8?B?ZVNySlNBQmNLcEFHc1kvU3JWWkVzUlRuVlFoYUJSSUx4OVk5Zjg4QUhIRG9Y?=
 =?utf-8?B?U2Y0S3NXTkFuR3BacFg1ekNTSDNVeFJoMFZKRUNrV0ZrTFpZTTB1cVFDYzl6?=
 =?utf-8?B?UUdkMjZzTVdWenhsTXFUY1IvaTVsRzI3WUdNcDNDYWFBdUtHTmF3dG53endX?=
 =?utf-8?B?R3IwZHNWOEdRWGtQTWkyeUpEeVc5Tk9ZZ3djN1hBN1k3djBFSXpnSWZLSjRo?=
 =?utf-8?B?clh2ZGZicGhIYkdxSzVCWFRNQS8xZHF3WWd6K2JyZnlMNlFiQThLd2l2alRj?=
 =?utf-8?B?d3FkQ3l3dlZCaFlITFY3NldVUnhxWUxXVHhsdVlvcGhRbU45MzJmQ2NPYm9o?=
 =?utf-8?B?dUJzQlA2czNlUVdUMGVSK2lNWWdMYnJwSzVTLzJTVWk5NE5PdU4xWGY2N3lR?=
 =?utf-8?B?OC85RHZVcDVZU2tUMTNmOHFGZ21KY3BVR0swVStKN0dmVHdWMjE3a3FCY2x1?=
 =?utf-8?B?RldpRlJieFFHRU5BWWg0SWFhcDVXeVFLK3FRTG85OGk0ZDRkRTZZdkxTNW9K?=
 =?utf-8?B?bUFhb1gzSnB0QzRWSVVhVU8zTERvTnphVm5Yd1U4N05YQ0NWS29aUGM1Q2pr?=
 =?utf-8?B?dmxwUW40SHFVais2NUdhVGl4TG8wUkF5d09UOEpXQjBKYmUxby9KYXhGMkpZ?=
 =?utf-8?B?MnhQd0FjUXpGL1FieDFzSmtRdmFWbS80REtrTnlZM1JaRGwrZnZUZFY1TVRt?=
 =?utf-8?B?UjFhUHJlREFudEFyNHBORW80djdGRHZIR01YSndCYzkyOVBFaXZZcVZVdEY5?=
 =?utf-8?B?U2ZOYU5GZysyUFFuQzcvWDZINktzU3pOZ2VJRDJucU01aDNTR21VN3l6dFdr?=
 =?utf-8?B?YStoQjMxcGdhWVM3ckVBVStINnhtNzhSZHp6T2lNTHdvUFJYaUVCNHlrUHRv?=
 =?utf-8?B?L2Q0cE1HeVJWYUd4YmdkUFZmWlYycFRybFVuM2x5ekFhdGtJTFByNGRpd1ZX?=
 =?utf-8?B?NVJxcXU4V2liOTR5eHVka2cxcnJ2ZmlrZ1BnT2dqTkQrNVdHMXVIOUhtNDYr?=
 =?utf-8?B?cTVpSTdPbWFlbDBoc1NGYnhjcUY5Tm5MNmIwN3hmN2VZV21SeEUvVC9lSitE?=
 =?utf-8?B?TFFVSVJuc3Z1UjV3enIzSG9XTnVpck95R3kvT2R2UWE4VEJYK1BpSzAyNzZK?=
 =?utf-8?B?bFFlcFNmMDd4VFVScmY4amNuL3NJR28vMEVCdlc0VXdqL2RkSktaSzFqTzg5?=
 =?utf-8?B?YyszMU5oZlZaZmVoRUFZSFFEOTdpMkN2UU1mTkx6b3FZbVRxVWFSeGtKMGVX?=
 =?utf-8?B?eEs1c0s1cnRGd3JpMkxrUGxGTlZpYnVOaFZPcFJsbU5UT1B4WnJxblo3dHR1?=
 =?utf-8?B?RDNwYmdOWHEyMGQwY2tPQzJBRU1BOEd2ekQwOE5sWVA4WDFvTGNUYk5GMDZV?=
 =?utf-8?B?elp3c1FKMnV0S3lhekVoOFNIclAyN2hmL2JvMzJHMnRiUDI0MVo5RWRuUmxy?=
 =?utf-8?B?TmtqWUpGZDQxWnk5RitHMHFsbW00bnhrbFFoemJ1VTZlMTZPenVFaDlnVGFC?=
 =?utf-8?B?ZjVUeFVMQ0lZSXo0RVZSbG4yVGt5MVAvQ2tsanorNW1QUDN5YmFORnM3cVlS?=
 =?utf-8?B?bW9ZeVdMemVFUVJvSVdPYTQ5cS9GM2ZYYXpicFh4a0pwM21HNkp4bXBXM05Z?=
 =?utf-8?B?aUNZNmp4WkVwcVlIWXFLa0d5Tngyb0dZeEZsckNyd2E3KzA5cDk2ejBGMFZn?=
 =?utf-8?Q?W+kzA6rUuCGzE0FlSa15O2CJJ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f5b676a-2782-48bd-3d45-08dc7e22c561
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5685.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 May 2024 07:58:18.6236
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gbgG/lT87xVDKnbwVWtHauqsKUbPTGOqhOQbeh/luDaVK6QoYToY2EKoQ0OJGg6y
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7881

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@gmail.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;
>   }
>