[PATCH v3] drm/amdkfd: Ignored various return code

[PATCH v3] drm/amdkfd: Ignored various return code

[Andrew Martin]

The return code of a non void function should not be ignored. In cases
where we do not care, the code needs to suppress it.

Signed-off-by: Andrew Martin <andrew.martin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 13 ++++++++-----
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c   |  3 ++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 644f79f3c9af..e4438fca6283 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -313,8 +313,10 @@ int amdgpu_amdkfd_post_reset(struct amdgpu_device *adev)
 void amdgpu_amdkfd_gpu_reset(struct amdgpu_device *adev)
 {
 	if (amdgpu_device_should_recover_gpu(adev))
-		amdgpu_reset_domain_schedule(adev->reset_domain,
-					     &adev->kfd.reset_work);
+		WARN_ONCE(!amdgpu_reset_domain_schedule(adev->reset_domain,
+							&adev->kfd.reset_work),
+							"Failed to queue work! at %s",
+							 __func__);
 }
 
 int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t size,
@@ -715,9 +717,10 @@ void amdgpu_amdkfd_set_compute_idle(struct amdgpu_device *adev, bool idle)
 		if (gfx_block != NULL)
 			gfx_block->version->funcs->set_powergating_state((void *)gfx_block, state);
 	}
-	amdgpu_dpm_switch_power_profile(adev,
-					PP_SMC_POWER_PROFILE_COMPUTE,
-					!idle);
+	WARN_ONCE(!amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_COMPUTE, !idle),
+		  "(%s) failed to disable video power profile mode",
+		  __func__);
+
 }
 
 bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 22925df6a791..025609a4abcf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2780,7 +2780,8 @@ static int runtime_enable(struct kfd_process *p, uint64_t r_debug,
 		 * saved in MES.
 		 */
 		if (pdd->dev->kfd->shared_resources.enable_mes)
-			kfd_dbg_set_mes_debug_mode(pdd, !kfd_dbg_has_cwsr_workaround(pdd->dev));
+			(void)kfd_dbg_set_mes_debug_mode(pdd,
+							 !kfd_dbg_has_cwsr_workaround(pdd->dev));
 	}
 
 	p->runtime_info.runtime_state = DEBUG_RUNTIME_STATE_ENABLED;
-- 
2.43.0

Re: [PATCH v3] drm/amdkfd: Ignored various return code

[Philip Yang]

On 2025-11-13 13:31, Andrew Martin wrote:
> The return code of a non void function should not be ignored. In cases
> where we do not care, the code needs to suppress it.
>
> Signed-off-by: Andrew Martin <andrew.martin@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 13 ++++++++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c   |  3 ++-
>   2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 644f79f3c9af..e4438fca6283 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -313,8 +313,10 @@ int amdgpu_amdkfd_post_reset(struct amdgpu_device *adev)
>   void amdgpu_amdkfd_gpu_reset(struct amdgpu_device *adev)
>   {
>   	if (amdgpu_device_should_recover_gpu(adev))
> -		amdgpu_reset_domain_schedule(adev->reset_domain,
> -					     &adev->kfd.reset_work);
> +		WARN_ONCE(!amdgpu_reset_domain_schedule(adev->reset_domain,
> +							&adev->kfd.reset_work),
> +							"Failed to queue work! at %s",
> +							 __func__);
amdgpu_reset_domain_schedule only return false if reset_work already 
exist in the workqueue, it is fine to ignore the return value, don't 
want WARN_ONCE for this.
>   }
>   
>   int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t size,
> @@ -715,9 +717,10 @@ void amdgpu_amdkfd_set_compute_idle(struct amdgpu_device *adev, bool idle)
>   		if (gfx_block != NULL)
>   			gfx_block->version->funcs->set_powergating_state((void *)gfx_block, state);
>   	}
> -	amdgpu_dpm_switch_power_profile(adev,
> -					PP_SMC_POWER_PROFILE_COMPUTE,
> -					!idle);
> +	WARN_ONCE(!amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_COMPUTE, !idle),
> +		  "(%s) failed to disable video power profile mode",
> +		  __func__);

amdgpu_dpm_switch_power_profile return false for SRIOV, 
pp_funcs->switch_power_profile may return false for many cases, like if 
(!hwmgr || !hwmgr->pm_en), I think we don't need WARN for those cases.

> +
>   }
>   
>   bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 22925df6a791..025609a4abcf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -2780,7 +2780,8 @@ static int runtime_enable(struct kfd_process *p, uint64_t r_debug,
>   		 * saved in MES.
>   		 */
>   		if (pdd->dev->kfd->shared_resources.enable_mes)
> -			kfd_dbg_set_mes_debug_mode(pdd, !kfd_dbg_has_cwsr_workaround(pdd->dev));
> +			(void)kfd_dbg_set_mes_debug_mode(pdd,
> +							 !kfd_dbg_has_cwsr_workaround(pdd->dev));

This looks good. It is easier to review if one patch only fix one 
ignored return code issue.

Regards,

Philip

>   	}
>   
>   	p->runtime_info.runtime_state = DEBUG_RUNTIME_STATE_ENABLED;

RE: [PATCH v3] drm/amdkfd: Ignored various return code

[Martin, Andrew]

[AMD Official Use Only - AMD Internal Distribution Only]

Greetings @Yang, Philip

> This looks good. It is easier to review if one patch only fix one ignored return code issue.

Thanks for the useful comments. The next patch(es) will remove the WARN_ONCE and the user "(void)" to suppress the unneeded return code.    I will also  break up the next update into 3 patches (1 for each  ignore ).

> -----Original Message-----
> From: Yang, Philip <Philip.Yang@amd.com>
> Sent: Friday, November 14, 2025 1:53 PM
> To: Martin, Andrew <Andrew.Martin@amd.com>; amd-
> gfx@lists.freedesktop.org
> Subject: Re: [PATCH v3] drm/amdkfd: Ignored various return code
>
>
> On 2025-11-13 13:31, Andrew Martin wrote:
> > The return code of a non void function should not be ignored. In cases
> > where we do not care, the code needs to suppress it.
> >
> > Signed-off-by: Andrew Martin <andrew.martin@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 13 ++++++++-----
> >   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c   |  3 ++-
> >   2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > index 644f79f3c9af..e4438fca6283 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > @@ -313,8 +313,10 @@ int amdgpu_amdkfd_post_reset(struct
> amdgpu_device *adev)
> >   void amdgpu_amdkfd_gpu_reset(struct amdgpu_device *adev)
> >   {
> >     if (amdgpu_device_should_recover_gpu(adev))
> > -           amdgpu_reset_domain_schedule(adev->reset_domain,
> > -                                        &adev->kfd.reset_work);
> > +           WARN_ONCE(!amdgpu_reset_domain_schedule(adev-
> >reset_domain,
> > +                                                   &adev-
> >kfd.reset_work),
> > +                                                   "Failed to queue work!
> at %s",
> > +                                                    __func__);
> amdgpu_reset_domain_schedule only return false if reset_work already exist in
> the workqueue, it is fine to ignore the return value, don't want WARN_ONCE
> for this.
> >   }
> >
> >   int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t
> > size, @@ -715,9 +717,10 @@ void amdgpu_amdkfd_set_compute_idle(struct
> amdgpu_device *adev, bool idle)
> >             if (gfx_block != NULL)
> >                     gfx_block->version->funcs-
> >set_powergating_state((void *)gfx_block, state);
> >     }
> > -   amdgpu_dpm_switch_power_profile(adev,
> > -                                   PP_SMC_POWER_PROFILE_COMPUTE,
> > -                                   !idle);
> > +   WARN_ONCE(!amdgpu_dpm_switch_power_profile(adev,
> PP_SMC_POWER_PROFILE_COMPUTE, !idle),
> > +             "(%s) failed to disable video power profile mode",
> > +             __func__);
>
> amdgpu_dpm_switch_power_profile return false for SRIOV, pp_funcs-
> >switch_power_profile may return false for many cases, like if (!hwmgr ||
> !hwmgr->pm_en), I think we don't need WARN for those cases.
>
> > +
> >   }
> >
> >   bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid)
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > index 22925df6a791..025609a4abcf 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > @@ -2780,7 +2780,8 @@ static int runtime_enable(struct kfd_process *p,
> uint64_t r_debug,
> >              * saved in MES.
> >              */
> >             if (pdd->dev->kfd->shared_resources.enable_mes)
> > -                   kfd_dbg_set_mes_debug_mode(pdd,
> !kfd_dbg_has_cwsr_workaround(pdd->dev));
> > +                   (void)kfd_dbg_set_mes_debug_mode(pdd,
> > +
> !kfd_dbg_has_cwsr_workaround(pdd->dev));
>
> This looks good. It is easier to review if one patch only fix one ignored return
> code issue.
>
> Regards,
>
> Philip
>
> >     }
> >
> >     p->runtime_info.runtime_state = DEBUG_RUNTIME_STATE_ENABLED;

Re: [PATCH v3] drm/amdkfd: Ignored various return code

[Felix Kuehling]

On 2025-11-13 13:31, Andrew Martin wrote:
> The return code of a non void function should not be ignored. In cases
> where we do not care, the code needs to suppress it.
>
> Signed-off-by: Andrew Martin <andrew.martin@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 13 ++++++++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c   |  3 ++-
>   2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 644f79f3c9af..e4438fca6283 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -313,8 +313,10 @@ int amdgpu_amdkfd_post_reset(struct amdgpu_device *adev)
>   void amdgpu_amdkfd_gpu_reset(struct amdgpu_device *adev)
>   {
>   	if (amdgpu_device_should_recover_gpu(adev))
> -		amdgpu_reset_domain_schedule(adev->reset_domain,
> -					     &adev->kfd.reset_work);
> +		WARN_ONCE(!amdgpu_reset_domain_schedule(adev->reset_domain,
> +							&adev->kfd.reset_work),
> +							"Failed to queue work! at %s",
> +							 __func__);

The return value is not an error. It only indicates that reset work was 
already queued.


>   }
>   
>   int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t size,
> @@ -715,9 +717,10 @@ void amdgpu_amdkfd_set_compute_idle(struct amdgpu_device *adev, bool idle)
>   		if (gfx_block != NULL)
>   			gfx_block->version->funcs->set_powergating_state((void *)gfx_block, state);
>   	}
> -	amdgpu_dpm_switch_power_profile(adev,
> -					PP_SMC_POWER_PROFILE_COMPUTE,
> -					!idle);
> +	WARN_ONCE(!amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_COMPUTE, !idle),
> +		  "(%s) failed to disable video power profile mode",
> +		  __func__);

I think 0 means success here. But I agree with Philip that we can 
probably ignore errors here. I believe some chips just don't support 
power profiles and return an error here.


> +
>   }
>   
>   bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 22925df6a791..025609a4abcf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -2780,7 +2780,8 @@ static int runtime_enable(struct kfd_process *p, uint64_t r_debug,
>   		 * saved in MES.
>   		 */
>   		if (pdd->dev->kfd->shared_resources.enable_mes)
> -			kfd_dbg_set_mes_debug_mode(pdd, !kfd_dbg_has_cwsr_workaround(pdd->dev));
> +			(void)kfd_dbg_set_mes_debug_mode(pdd,
> +							 !kfd_dbg_has_cwsr_workaround(pdd->dev));

In this case we could actually handle the error and return an error up 
to the caller.

Regards,
   Felix


>   	}
>   
>   	p->runtime_info.runtime_state = DEBUG_RUNTIME_STATE_ENABLED;