[PATCH v2] drm/amdkfd: Fix stack size in 'amdgpu_amdkfd_unmap_hiq'

[Chen, Guchun]

[Public]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Srinivasan Shanmugam
> Sent: Sunday, July 9, 2023 11:05 PM
> To: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>
> Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>;
> amd-gfx at lists.freedesktop.org
> 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 at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: winmail.dat
Type: application/ms-tnef
Size: 16539 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230710/1fc80c66/attachment-0001.bin>