RE: [PATCH 10/12] drm/amdkfd: remove dead code in kq_initialize

[Zhang, Jesse(Jie)]

[AMD Official Use Only - AMD Internal Distribution Only]

HI Felix

-----Original Message-----
From: Kuehling, Felix <felix.kuehl...@amd.com>
Sent: Wednesday, June 5, 2024 2:45 AM
To: Zhang, Jesse(Jie) <jesse.zh...@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; Huang, Tim <tim.hu...@amd.com>
Subject: Re: [PATCH 10/12] drm/amdkfd: remove dead code in kq_initialize


On 2024-06-03 04:49, Jesse Zhang wrote:
> The queue type can only be KFD_QUEUE_TYPE_DIQ or KFD_QUEUE_TYPE_HIQ,
> and the default cannot be reached.

I wonder, if you remove the default case, I guess you are relying on the 
compiler or a static checker to ensure that we can only pass valid enum values 
to this function. I don't think C compilers are that strict. You could pass a 
random integer to the function. That said, this function only has two callers, 
and both of them use a proper enum value.

[Zhang, Jesse(Jie)]  At the beginning of the function kq_initialize, we check 
the queue type that can ensure the  warning about C compilers.

static bool kq_initialize(struct kernel_queue *kq, struct kfd_node *dev,
                enum kfd_queue_type type, unsigned int queue_size)
{
        struct queue_properties prop;
        int retval;
        union PM4_MES_TYPE_3_HEADER nop;

        if (WARN_ON(type != KFD_QUEUE_TYPE_DIQ && type != KFD_QUEUE_TYPE_HIQ))
                return false;
             ...
}

Thanks
Jesse
>
> Signed-off-by: Jesse Zhang <jesse.zh...@amd.com>

Acked-by: Felix Kuehling <felix.kuehl...@amd.com>


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> index 32c926986dbb..3142b2593e2b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> @@ -67,9 +67,6 @@ static bool kq_initialize(struct kernel_queue *kq, struct 
> kfd_node *dev,
>       case KFD_QUEUE_TYPE_HIQ:
>               kq->mqd_mgr = dev->dqm->mqd_mgrs[KFD_MQD_TYPE_HIQ];
>               break;
> -     default:
> -             pr_err("Invalid queue type %d\n", type);
> -             return false;
>       }
>
>       if (!kq->mqd_mgr)