Re: [PATCH v4 07/15] drm/amdgpu: validate resume before function call

["Khatri, Sunil" <sunil.khatri@xxxxxxx>]

On 10/17/2024 5:25 PM, Christian König wrote:
> Same comment as patch #5, please add an amdgpu_ip_block_resume() 
> function.
Sure
> Regards,
> Christian.
>
> Am 17.10.24 um 12:06 schrieb Sunil Khatri:
> > Before making a function call to resume, validate
> > the function pointer like we do in sw_init.
> >
> > Signed-off-by: Sunil Khatri <sunil.khatri@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/aldebaran.c      | 23 ++++++----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 47 ++++++++++++---------
> >   drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c | 28 ++++++------
> >   drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c   | 14 +++---
> >   4 files changed, 66 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/aldebaran.c 
> > b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
> > index e55d680d95ce..4dd9af51210d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
> > @@ -247,9 +247,11 @@ static int aldebaran_mode2_restore_ip(struct 
> > amdgpu_device *adev)
> >           dev_err(adev->dev, "Failed to get BIF handle\n");
> >           return -EINVAL;
> >       }
> > -    r = cmn_block->version->funcs->resume(cmn_block);
> > -    if (r)
> > -        return r;
> > +    if (cmn_block->version->funcs->resume) {
> > +        r = cmn_block->version->funcs->resume(cmn_block);
> > +        if (r)
> > +            return r;
> > +    }
> >         /* Reinit GFXHUB */
> >       adev->gfxhub.funcs->init(adev);
> > @@ -283,12 +285,15 @@ static int aldebaran_mode2_restore_ip(struct 
> > amdgpu_device *adev)
> >                 adev->ip_blocks[i].version->type ==
> >                     AMD_IP_BLOCK_TYPE_SDMA))
> >               continue;
> > -        r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -        if (r) {
> > -            dev_err(adev->dev,
> > -                "resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -            return r;
> > +
> > +        if (adev->ip_blocks[i].version->funcs->resume) {
> > +            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +            if (r) {
> > +                dev_err(adev->dev,
> > +                    "resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                return r;
> > +            }
> >           }
> >             adev->ip_blocks[i].status.hw = true;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index fda89aeb4d1b..aa7314c14db8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2744,11 +2744,15 @@ static int amdgpu_device_fw_loading(struct 
> > amdgpu_device *adev)
> >                   break;
> >                 if (amdgpu_in_reset(adev) || adev->in_suspend) {
> > -                r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -                if (r) {
> > -                    DRM_ERROR("resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -                    return r;
> > +                if (adev->ip_blocks[i].version->funcs->resume) {
> > +                    r = adev->ip_blocks[i].version->funcs->resume(
> > +                        &adev->ip_blocks[i]);
> > +                    if (r) {
> > +                        DRM_ERROR("resume of IP block <%s> failed 
> > %d\n",
> > + adev->ip_blocks[i].version->funcs
> > +                            ->name, r);
> > +                        return r;
> > +                    }
> >                   }
> >               } else {
> >                   r = 
> > adev->ip_blocks[i].version->funcs->hw_init(&adev->ip_blocks[i]);
> > @@ -3683,10 +3687,12 @@ static int 
> > amdgpu_device_ip_reinit_late_sriov(struct amdgpu_device *adev)
> >                   block->status.hw)
> >                   continue;
> >   -            if (block->version->type == AMD_IP_BLOCK_TYPE_SMC)
> > -                r = block->version->funcs->resume(&adev->ip_blocks[i]);
> > -            else
> > +            if (block->version->type == AMD_IP_BLOCK_TYPE_SMC) {
> > +                if (adev->ip_blocks[i].version->funcs->resume)
> > +                    r = 
> > block->version->funcs->resume(&adev->ip_blocks[i]);
> > +            } else {
> >                   r = 
> > block->version->funcs->hw_init(&adev->ip_blocks[i]);
> > +            }
> >                 DRM_INFO("RE-INIT-late: %s %s\n", 
> > block->version->funcs->name, r?"failed":"succeeded");
> >               if (r)
> > @@ -3721,12 +3727,13 @@ static int 
> > amdgpu_device_ip_resume_phase1(struct amdgpu_device *adev)
> >               adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_GMC ||
> >               adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_IH ||
> >               (adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_PSP && amdgpu_sriov_vf(adev))) {
> > -
> > -            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -            if (r) {
> > -                DRM_ERROR("resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -                return r;
> > +            if (adev->ip_blocks[i].version->funcs->resume) {
> > +                r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +                if (r) {
> > +                    DRM_ERROR("resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                    return r;
> > +                }
> >               }
> >               adev->ip_blocks[i].status.hw = true;
> >           }
> > @@ -3760,11 +3767,13 @@ static int 
> > amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
> >               adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_IH ||
> >               adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_PSP)
> >               continue;
> > -        r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -        if (r) {
> > -            DRM_ERROR("resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -            return r;
> > +        if (adev->ip_blocks[i].version->funcs->resume) {
> > +            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +            if (r) {
> > +                DRM_ERROR("resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                return r;
> > +            }
> >           }
> >           adev->ip_blocks[i].status.hw = true;
> >       }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c 
> > b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
> > index 10dece12509f..082a69babe62 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
> > @@ -176,12 +176,14 @@ static int 
> > sienna_cichlid_mode2_restore_ip(struct amdgpu_device *adev)
> >         for (i = 0; i < adev->num_ip_blocks; i++) {
> >           if (adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_IH) {
> > -            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -            if (r) {
> > -                dev_err(adev->dev,
> > -                    "resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -                return r;
> > +            if (adev->ip_blocks[i].version->funcs->resume) {
> > +                r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +                if (r) {
> > +                    dev_err(adev->dev,
> > +                        "resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                    return r;
> > +                }
> >               }
> >                 adev->ip_blocks[i].status.hw = true;
> > @@ -194,12 +196,14 @@ static int 
> > sienna_cichlid_mode2_restore_ip(struct amdgpu_device *adev)
> >                 adev->ip_blocks[i].version->type ==
> >                     AMD_IP_BLOCK_TYPE_SDMA))
> >               continue;
> > -        r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -        if (r) {
> > -            dev_err(adev->dev,
> > -                "resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -            return r;
> > +        if (adev->ip_blocks[i].version->funcs->resume) {
> > +            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +            if (r) {
> > +                dev_err(adev->dev,
> > +                    "resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                return r;
> > +            }
> >           }
> >             adev->ip_blocks[i].status.hw = true;
> > 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 ab049f0b4d39..2e041424ef9f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
> > @@ -187,12 +187,14 @@ static int smu_v13_0_10_mode2_restore_ip(struct 
> > amdgpu_device *adev)
> >                 adev->ip_blocks[i].version->type ==
> >                     AMD_IP_BLOCK_TYPE_SDMA))
> >               continue;
> > -        r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > -        if (r) {
> > -            dev_err(adev->dev,
> > -                "resume of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > -            return r;
> > +        if (adev->ip_blocks[i].version->funcs->resume) {
> > +            r = 
> > adev->ip_blocks[i].version->funcs->resume(&adev->ip_blocks[i]);
> > +            if (r) {
> > +                dev_err(adev->dev,
> > +                    "resume of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                return r;
> > +            }
> >           }
> >             adev->ip_blocks[i].status.hw = true;