[Public]
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Srinivasan Shanmugam
> Sent: Sunday, July 9, 2023 11:05 PM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH v2] drm/amdkfd: Fix stack size in
> 'amdgpu_amdkfd_unmap_hiq'
>
> Allocate large local variable on heap to avoid exceeding the stack size:
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c: In function
> ‘amdgpu_amdkfd_unmap_hiq’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:868:1: warning: the
> frame size of 1280 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 34 +++++++++++++++---
> ----
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 0040c63e2356..8c9a48062abf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -836,33 +836,45 @@ int amdgpu_amdkfd_unmap_hiq(struct
> amdgpu_device *adev, u32 doorbell_off, {
> struct amdgpu_kiq *kiq = &adev->gfx.kiq[inst];
> struct amdgpu_ring *kiq_ring = &kiq->ring;
> - struct amdgpu_ring_funcs ring_funcs;
> - struct amdgpu_ring ring;
> + struct amdgpu_ring_funcs *ring_funcs;
> + struct amdgpu_ring *ring;
> int r = 0;
>
> - if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> - return -EINVAL;
> + ring_funcs = kzalloc(sizeof(*ring_funcs), GFP_KERNEL);
> + if (!ring_funcs)
> + return -ENOMEM;
>
> - memset(&ring, 0x0, sizeof(struct amdgpu_ring));
> - memset(&ring_funcs, 0x0, sizeof(struct amdgpu_ring_funcs));
> + ring = kzalloc(sizeof(*ring), GFP_KERNEL);
> + if (!ring)
> + return -ENOMEM;
Should it free ring_funcs before 'return -ENOMEM'?
> - ring_funcs.type = AMDGPU_RING_TYPE_COMPUTE;
> - ring.doorbell_index = doorbell_off;
> - ring.funcs = &ring_funcs;
> + if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues) {
> + r = -EINVAL;
> + goto free_ring;
> + }
It's suggested to keep the check of ' kiq->pmf' and 'kiq->pmf->kiq_unmap_queues' ahead of allocation of ring & ring_funcs.
Regards,
Guchun
> + ring_funcs->type = AMDGPU_RING_TYPE_COMPUTE;
> + ring->doorbell_index = doorbell_off;
> + ring->funcs = ring_funcs;
>
> spin_lock(&kiq->ring_lock);
>
> if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> spin_unlock(&kiq->ring_lock);
> - return -ENOMEM;
> + r = -ENOMEM;
> + goto free_ring;
> }
>
> - kiq->pmf->kiq_unmap_queues(kiq_ring, &ring, RESET_QUEUES, 0, 0);
> + kiq->pmf->kiq_unmap_queues(kiq_ring, ring, RESET_QUEUES, 0, 0);
>
> if (kiq_ring->sched.ready && !adev->job_hang)
> r = amdgpu_ring_test_helper(kiq_ring);
>
> spin_unlock(&kiq->ring_lock);
>
> +free_ring:
> + kfree(ring_funcs);
> + kfree(ring);
> +
> return r;
> }
> --
> 2.25.1
<<attachment: winmail.dat>>
