[PATCH 0/1] amdgpu: Fix crash when MEC2 firmware fails to load

[PATCH 0/1] amdgpu: Fix crash when MEC2 firmware fails to load

[David McFarland]

I only noticed this because my distro broke the mec2.bin symbolic link, so the
firmware failed to load.  This resulted in:

amdgpu 0000:0b:00.0: Direct firmware load for amdgpu/navi10_mec2.bin failed with error -2
BUG: kernel NULL pointer dereference, address: 0000000000000008
[...]
Call Trace:
 <TASK>
 ? __die+0x23/0x70
 ? page_fault_oops+0x17d/0x4b0
 ? srso_alias_return_thunk+0x5/0xfbef5
 ? exc_page_fault+0x72/0x160
 ? asm_exc_page_fault+0x26/0x30
 ? request_firmware+0x48/0x60 [firmware_class]
 ? amdgpu_gfx_cp_init_microcode+0x22e/0x260 [amdgpu]
 gfx_v10_0_early_init+0x722/0x8d0 [amdgpu]

The attached patch removes some code that looks like it was added by mistake, or
by a bad merge. With it I get still get the load failed error as expected, but
no crash. After fixing the firmware, no error is printed.


David McFarland (1):
  drm/amd: Don't init MEC2 firmware when it fails to load

 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
 1 file changed, 2 deletions(-)

-- 
2.40.1

[PATCH 1/1] drm/amd: Don't init MEC2 firmware when it fails to load

[David McFarland]

The same calls are made directly above, but conditional on the firmware
loading and validating successfully.

Fixes: 9931b67690cf ("drm/amd: Load GFX10 microcode during early_init")
---
 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index d63cab294883..b0ba68016a02 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4022,18 +4022,16 @@ static int gfx_v10_0_init_microcode(struct amdgpu_device *adev)
 	err = amdgpu_ucode_request(adev, &adev->gfx.mec2_fw, fw_name);
 	if (!err) {
 		amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
 		amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
 	} else {
 		err = 0;
 		adev->gfx.mec2_fw = NULL;
 	}
-	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
-	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
 
 	gfx_v10_0_check_fw_write_wait(adev);
 out:
 	if (err) {
 		amdgpu_ucode_release(&adev->gfx.pfp_fw);
 		amdgpu_ucode_release(&adev->gfx.me_fw);
 		amdgpu_ucode_release(&adev->gfx.ce_fw);
 		amdgpu_ucode_release(&adev->gfx.rlc_fw);
-- 
2.40.1

Re: [PATCH 1/1] drm/amd: Don't init MEC2 firmware when it fails to load

[Mario Limonciello]

On 1/28/2024 10:18, David McFarland wrote:
> The same calls are made directly above, but conditional on the firmware
> loading and validating successfully.
> 
> Fixes: 9931b67690cf ("drm/amd: Load GFX10 microcode during early_init")

This is a great find, thank you for the fix.  I'll apply it to our tree 
and Alex should send it in a future fixes PR.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index d63cab294883..b0ba68016a02 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4022,18 +4022,16 @@ static int gfx_v10_0_init_microcode(struct amdgpu_device *adev)
>   	err = amdgpu_ucode_request(adev, &adev->gfx.mec2_fw, fw_name);
>   	if (!err) {
>   		amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
>   		amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
>   	} else {
>   		err = 0;
>   		adev->gfx.mec2_fw = NULL;
>   	}
> -	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
> -	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
>   
>   	gfx_v10_0_check_fw_write_wait(adev);
>   out:
>   	if (err) {
>   		amdgpu_ucode_release(&adev->gfx.pfp_fw);
>   		amdgpu_ucode_release(&adev->gfx.me_fw);
>   		amdgpu_ucode_release(&adev->gfx.ce_fw);
>   		amdgpu_ucode_release(&adev->gfx.rlc_fw);

Re: [PATCH 1/1] drm/amd: Don't init MEC2 firmware when it fails to load

[Mario Limonciello]

On 1/29/2024 14:46, Mario Limonciello wrote:
> On 1/28/2024 10:18, David McFarland wrote:
>> The same calls are made directly above, but conditional on the firmware
>> loading and validating successfully.
>>
>> Fixes: 9931b67690cf ("drm/amd: Load GFX10 microcode during early_init")
> 
> This is a great find, thank you for the fix.  I'll apply it to our tree 
> and Alex should send it in a future fixes PR.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Sorry to be pedantic; but I realized after I tried to apply this is 
missing a S-o-b.  Can you please add one?

> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> index d63cab294883..b0ba68016a02 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> @@ -4022,18 +4022,16 @@ static int gfx_v10_0_init_microcode(struct 
>> amdgpu_device *adev)
>>       err = amdgpu_ucode_request(adev, &adev->gfx.mec2_fw, fw_name);
>>       if (!err) {
>>           amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
>>           amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
>>       } else {
>>           err = 0;
>>           adev->gfx.mec2_fw = NULL;
>>       }
>> -    amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
>> -    amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
>>       gfx_v10_0_check_fw_write_wait(adev);
>>   out:
>>       if (err) {
>>           amdgpu_ucode_release(&adev->gfx.pfp_fw);
>>           amdgpu_ucode_release(&adev->gfx.me_fw);
>>           amdgpu_ucode_release(&adev->gfx.ce_fw);
>>           amdgpu_ucode_release(&adev->gfx.rlc_fw);
> 

[PATCH v2 0/1] drm/amd: Don't init MEC2 firmware when it fails to load

[David McFarland]

> Sorry to be pedantic; but I realized after I tried to apply this is
> missing a S-o-b.  Can you please add one?

Of course, here you go.  I left it off because I wasn't 100% sure about
the intention of your previous change.


David McFarland (1):
  drm/amd: Don't init MEC2 firmware when it fails to load

 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
 1 file changed, 2 deletions(-)

-- 
2.40.1

[PATCH v2 1/1] drm/amd: Don't init MEC2 firmware when it fails to load

[David McFarland]

The same calls are made directly above, but conditional on the firmware
loading and validating successfully.

Fixes: 9931b67690cf ("drm/amd: Load GFX10 microcode during early_init")
Signed-off-by: David McFarland <corngood@gmail.com>
---
v2: signed off

 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index d63cab294883..b0ba68016a02 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4027,8 +4027,6 @@ static int gfx_v10_0_init_microcode(struct amdgpu_device *adev)
 		err = 0;
 		adev->gfx.mec2_fw = NULL;
 	}
-	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
-	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
 
 	gfx_v10_0_check_fw_write_wait(adev);
 out:
-- 
2.40.1

Re: [PATCH v2 1/1] drm/amd: Don't init MEC2 firmware when it fails to load

[Mario Limonciello]

On 1/29/2024 16:18, David McFarland wrote:
> The same calls are made directly above, but conditional on the firmware
> loading and validating successfully.
> 
> Fixes: 9931b67690cf ("drm/amd: Load GFX10 microcode during early_init")
> Signed-off-by: David McFarland <corngood@gmail.com>
> ---
> v2: signed off
Thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

And I'll apply this now.

> 
>   drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index d63cab294883..b0ba68016a02 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4027,8 +4027,6 @@ static int gfx_v10_0_init_microcode(struct amdgpu_device *adev)
>   		err = 0;
>   		adev->gfx.mec2_fw = NULL;
>   	}
> -	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2);
> -	amdgpu_gfx_cp_init_microcode(adev, AMDGPU_UCODE_ID_CP_MEC2_JT);
>   
>   	gfx_v10_0_check_fw_write_wait(adev);
>   out: