[PATCH Review 1/1] drm/amdgpu/pm: add asic smu support check

[Paul Menzel]

Dear Stanley,


Thank you for your patch.


Am 21.03.22 um 06:45 schrieb Stanley.Yang:

Some nits:

Could you please remove the dot from the name:

     $ git config --global user.name "Stanley Yang"
     $ git commit --amend -s --author="Stanley Yang <Stanley.Yang at amd.com>"

The prefix drm/amd/pm seems to be more common for changes in the file. 
Instead of *add … check*, you can also just use the verb *check*:

drm/amd/pm: Check ASIC SMU support

> It must check asic whether support smu
> before call smu powerplay function, otherwise
> it may cause null point on no support smu asic.

Please reflow for 75 characters per line. Also maybe write:

… it may cause a null pointer dereference on systems without an SMU ASIC.

(I am no native speaker myself though.)

On what device did you reproduce this null pointer dereference?

> Change-Id: Ib86f3d4c88317b23eb1040b9ce1c5c8dcae42488

Without documenting the Gerrit instance, the Change-Id is of no use.

I guess this also should go to the stable series? Please add a Fixes tag 
with the commit missing these checks.

> Signed-off-by: Stanley.Yang <Stanley.Yang at amd.com>
> ---
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index 89fbee568be4..c73fb73e9628 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -500,6 +500,9 @@ int amdgpu_dpm_send_hbm_bad_pages_num(struct amdgpu_device *adev, uint32_t size)
>   	struct smu_context *smu = adev->powerplay.pp_handle;
>   	int ret = 0;
>   
> +	if (!is_support_sw_smu(adev))
> +		return -EOPNOTSUPP;
> +

I wonder, why `struct smu_context *smu = adev->powerplay.pp_handle` 
seems to work further up? Maybe the assignment should also be moved 
after the newly introduced check?

>   	mutex_lock(&adev->pm.mutex);
>   	ret = smu_send_hbm_bad_pages_num(smu, size);
>   	mutex_unlock(&adev->pm.mutex);
> @@ -512,6 +515,9 @@ int amdgpu_dpm_send_hbm_bad_channel_flag(struct amdgpu_device *adev, uint32_t si
>   	struct smu_context *smu = adev->powerplay.pp_handle;
>   	int ret = 0;
>   
> +	if (!is_support_sw_smu(adev))
> +		return -EOPNOTSUPP;
> +
>   	mutex_lock(&adev->pm.mutex);
>   	ret = smu_send_hbm_bad_channel_flag(smu, size);
>   	mutex_unlock(&adev->pm.mutex);


Kind regards,

Paul