[PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[Sunil Khatri]

An invalid return value 'r' of the pm_runtime_get_sync
is r < 0 so fix the return value and add proper logging
of failure  and exit cleanly.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 3a71d01b54c9..6babadeec268 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -289,8 +289,11 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 	queue->userq_prop = userq_props;
 
 	r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
-	if (r)
+	if (r < 0) {
+		DRM_ERROR("Failed to increment ref count of device pm_runtime \n");
+		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
 		goto free_mqd;
+	}
 
 	r = mqd_hw_default->init_mqd(adev, (void *)queue->mqd.cpu_ptr, userq_props);
 	if (r) {
-- 
2.34.1

Re: [PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[Mario Limonciello]

On 3/4/2025 11:51, Sunil Khatri wrote:
> An invalid return value 'r' of the pm_runtime_get_sync
> is r < 0 so fix the return value and add proper logging
> of failure  and exit cleanly.

You have an extra space between "failure" and "and".

> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 3a71d01b54c9..6babadeec268 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> @@ -289,8 +289,11 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>   	queue->userq_prop = userq_props;
>   
>   	r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
> -	if (r)
> +	if (r < 0) {
> +		DRM_ERROR("Failed to increment ref count of device pm_runtime \n");

For new code can you please use drm_err() instead of DRM_ERROR()?

> +		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>   		goto free_mqd;
> +	}
>   
>   	r = mqd_hw_default->init_mqd(adev, (void *)queue->mqd.cpu_ptr, userq_props);
>   	if (r) {

RE: [PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[Khatri, Sunil]

[AMD Official Use Only - AMD Internal Distribution Only]

-----Original Message-----
From: Limonciello, Mario <Mario.Limonciello@amd.com>
Sent: Tuesday, March 4, 2025 11:27 PM
To: Khatri, Sunil <Sunil.Khatri@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

On 3/4/2025 11:51, Sunil Khatri wrote:
> An invalid return value 'r' of the pm_runtime_get_sync is r < 0 so fix
> the return value and add proper logging of failure  and exit cleanly.

You have an extra space between "failure" and "and".
Noted.

>
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 3a71d01b54c9..6babadeec268 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> @@ -289,8 +289,11 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>       queue->userq_prop = userq_props;
>
>       r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
> -     if (r)
> +     if (r < 0) {
> +             DRM_ERROR("Failed to increment ref count of device pm_runtime \n");

For new code can you please use drm_err() instead of DRM_ERROR()?
I see drm_err is not used anywhere in amdgpu driver but display is using it. I think it would be better if I take it in a different patch to change instead. Does that sounds ok ?

Regards
Sunil Khatri

> +             pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>               goto free_mqd;
> +     }
>
>       r = mqd_hw_default->init_mqd(adev, (void *)queue->mqd.cpu_ptr, userq_props);
>       if (r) {

Re: [PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[Mario Limonciello]

On 3/4/2025 12:03, Khatri, Sunil wrote:
> For new code can you please use drm_err() instead of DRM_ERROR()?
> I see drm_err is not used anywhere in amdgpu driver but display is using it. I think it would be better if I take it in a different patch to change instead. Does that sounds ok ?
> 

DRM_ERROR() doesn't give any information on a multi-GPU system which GPU 
had the problem.  drm_err() does.  So it's the preferred macro for errors.

Up to Alex and Christian if they take this as is or spin it, but I think 
it's a very simple change.

> Regards
> Sunil Khatri
> 
>> +             pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>>                goto free_mqd;
>> +     }
>>
>>        r = mqd_hw_default->init_mqd(adev, (void *)queue->mqd.cpu_ptr, userq_props);
>>        if (r) {
> 

Re: [PATCH] drm/amdgpu: Validate return value of pm_runtime_get_sync

[Alex Deucher]

On Tue, Mar 4, 2025 at 1:14 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 3/4/2025 12:03, Khatri, Sunil wrote:
> > For new code can you please use drm_err() instead of DRM_ERROR()?
> > I see drm_err is not used anywhere in amdgpu driver but display is using it. I think it would be better if I take it in a different patch to change instead. Does that sounds ok ?
> >
>
> DRM_ERROR() doesn't give any information on a multi-GPU system which GPU
> had the problem.  drm_err() does.  So it's the preferred macro for errors.
>
> Up to Alex and Christian if they take this as is or spin it, but I think
> it's a very simple change.

Please use the dev_err() or drm_err() variants. We mostly use the dev
variants outside of the display code.

Alex

>
> > Regards
> > Sunil Khatri
> >
> >> +             pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> >>                goto free_mqd;
> >> +     }
> >>
> >>        r = mqd_hw_default->init_mqd(adev, (void *)queue->mqd.cpu_ptr, userq_props);
> >>        if (r) {
> >
>