[PATCH v1 06/13] drm/amdgpu: validate suspend before function call

[Khatri, Sunil]

On 10/10/2024 1:37 PM, Khatri, Sunil wrote:
>
> On 10/10/2024 1:13 PM, Christian König wrote:
>> Am 09.10.24 um 16:24 schrieb Sunil Khatri:
>>> Before making a function call to suspend, validate
>>> the function pointer like we do in sw_init.
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/aldebaran.c      | 15 ++++++------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 26 
>>> ++++++++++++---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c   | 12 ++++++----
>>>   drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c | 15 ++++++------
>>>   drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c   | 15 ++++++------
>>>   5 files changed, 46 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/aldebaran.c 
>>> b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
>>> index c1ff24335a0c..e55d680d95ce 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
>>> @@ -85,13 +85,14 @@ static int aldebaran_mode2_suspend_ip(struct 
>>> amdgpu_device *adev)
>>>                     AMD_IP_BLOCK_TYPE_SDMA))
>>>               continue;
>>>   -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -
>>> -        if (r) {
>>> -            dev_err(adev->dev,
>>> -                "suspend of IP block <%s> failed %d\n",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> -            return r;
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            if (r) {
>>> +                dev_err(adev->dev,
>>> +                    "suspend of IP block <%s> failed %d\n",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +                return r;
>>> +            }
>>
>> I think we should probably create a function for that code and error 
>> message repeated a number of times. Same for the resume function.
>
> Common static inline functions only for suspend and resume sounds good ??
Each of the functions where error message is logged are in different 
files and having no commons. Also its just one like print and i think 
its ok and rather avoid calling a separate function for just one line.

Even if i decide to create a dedication function it will be in either of 
andgpu_ring.h or amdgpu_reset.h    or amdgpu_job.h only which right now 
holds information specific to the header only.

what your suggestion here ?

>
> Regards
>
> Sunil khatri
>
>>
>> Apart from that the whole set looks good to me.
>>
>> Regards,
>> Christian.
>>
>>>           }
>>>             adev->ip_blocks[i].status.hw = false;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 340141a74c12..51607ac8adb9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3471,12 +3471,14 @@ static int 
>>> amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev)
>>>               continue;
>>>             /* XXX handle errors */
>>> -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -        /* XXX handle errors */
>>> -        if (r) {
>>> -            DRM_ERROR("suspend of IP block <%s> failed %d\n",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> -            return r;
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            /* XXX handle errors */
>>> +            if (r) {
>>> +                DRM_ERROR("suspend of IP block <%s> failed %d\n",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +                return r;
>>> +            }
>>>           }
>>>             adev->ip_blocks[i].status.hw = false;
>>> @@ -3553,11 +3555,13 @@ static int 
>>> amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
>>>               continue;
>>>             /* XXX handle errors */
>>> -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -        /* XXX handle errors */
>>> -        if (r) {
>>> -            DRM_ERROR("suspend of IP block <%s> failed %d\n",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            /* XXX handle errors */
>>> +            if (r) {
>>> +                DRM_ERROR("suspend of IP block <%s> failed %d\n",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +            }
>>>           }
>>>           adev->ip_blocks[i].status.hw = false;
>>>           /* handle putting the SMC in the appropriate state */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> index 3e2724590dbf..6bc75aa1c3b1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> @@ -40,11 +40,13 @@ static int 
>>> amdgpu_reset_xgmi_reset_on_init_suspend(struct amdgpu_device *adev)
>>>               continue;
>>>             /* XXX handle errors */
>>> -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -        /* XXX handle errors */
>>> -        if (r) {
>>> -            dev_err(adev->dev, "suspend of IP block <%s> failed %d",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            /* XXX handle errors */
>>> +            if (r) {
>>> +                dev_err(adev->dev, "suspend of IP block <%s> failed 
>>> %d",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +            }
>>>           }
>>>           adev->ip_blocks[i].status.hw = false;
>>>       }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c 
>>> b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
>>> index 475b7df3a908..10dece12509f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
>>> @@ -81,13 +81,14 @@ static int 
>>> sienna_cichlid_mode2_suspend_ip(struct amdgpu_device *adev)
>>>                     AMD_IP_BLOCK_TYPE_SDMA))
>>>               continue;
>>>   -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -
>>> -        if (r) {
>>> -            dev_err(adev->dev,
>>> -                "suspend of IP block <%s> failed %d\n",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> -            return r;
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            if (r) {
>>> +                dev_err(adev->dev,
>>> +                    "suspend of IP block <%s> failed %d\n",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +                return r;
>>> +            }
>>>           }
>>>           adev->ip_blocks[i].status.hw = false;
>>>       }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c 
>>> b/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
>>> index 5ea9090b5040..ab049f0b4d39 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
>>> @@ -80,13 +80,14 @@ static int smu_v13_0_10_mode2_suspend_ip(struct 
>>> amdgpu_device *adev)
>>>                     AMD_IP_BLOCK_TYPE_MES))
>>>               continue;
>>>   -        r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> -
>>> -        if (r) {
>>> -            dev_err(adev->dev,
>>> -                "suspend of IP block <%s> failed %d\n",
>>> - adev->ip_blocks[i].version->funcs->name, r);
>>> -            return r;
>>> +        if (adev->ip_blocks[i].version->funcs->suspend) {
>>> +            r = 
>>> adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
>>> +            if (r) {
>>> +                dev_err(adev->dev,
>>> +                    "suspend of IP block <%s> failed %d\n",
>>> + adev->ip_blocks[i].version->funcs->name, r);
>>> +                return r;
>>> +            }
>>>           }
>>>           adev->ip_blocks[i].status.hw = false;
>>>       }
>>