Re: [PATCH v4 05/15] drm/amdgpu: validate hw_fini before function call

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

On 10/17/2024 5:22 PM, Christian König wrote:
> Am 17.10.24 um 12:06 schrieb Sunil Khatri:
> > Before making a function call to hw_fini, validate
> > the function pointer like we do in sw_init.
> >
> > Signed-off-by: Sunil Khatri <sunil.khatri@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 ++++++++++++++--------
> >   1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index cf84b50f2355..96cecd4a636b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3287,11 +3287,17 @@ static void 
> > amdgpu_device_smu_fini_early(struct amdgpu_device *adev)
> >           if (!adev->ip_blocks[i].status.hw)
> >               continue;
> >           if (adev->ip_blocks[i].version->type == 
> > AMD_IP_BLOCK_TYPE_SMC) {
> > -            r = 
> > adev->ip_blocks[i].version->funcs->hw_fini(&adev->ip_blocks[i]);
> > -            /* XXX handle errors */
> > -            if (r) {
> > -                DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > +            if (!adev->ip_blocks[i].version->funcs->hw_fini) {
> > +                DRM_ERROR("hw_fini of IP block <%s> not defined\n",
> > + adev->ip_blocks[i].version->funcs->name);
> > +            } else {
> > +                r = adev->ip_blocks[i].version->funcs->hw_fini(
> > +                    &adev->ip_blocks[i]);
> > +                /* XXX handle errors */
> > +                if (r) {
> > +                    DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +                }
> >               }
> >               adev->ip_blocks[i].status.hw = false;
> >               break;
> > @@ -3325,12 +3331,17 @@ static int amdgpu_device_ip_fini_early(struct 
> > amdgpu_device *adev)
> >       for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
> >           if (!adev->ip_blocks[i].status.hw)
> >               continue;
> > -
> > -        r = 
> > adev->ip_blocks[i].version->funcs->hw_fini(&adev->ip_blocks[i]);
> > -        /* XXX handle errors */
> > -        if (r) {
> > -            DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> > - adev->ip_blocks[i].version->funcs->name, r);
> > +        if (!adev->ip_blocks[i].version->funcs->hw_fini) {
> > +            DRM_ERROR("hw_fini of IP block <%s> not defined\n",
> > + adev->ip_blocks[i].version->funcs->name);
> > +        } else {
> > +            r = adev->ip_blocks[i].version->funcs->hw_fini(
> > +                &adev->ip_blocks[i]);
> > +            /* XXX handle errors */
> > +            if (r) {
> > +                DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> > + adev->ip_blocks[i].version->funcs->name, r);
> > +            }
>
> I strongly suggest to make this a separate function, e.g. something 
> like amdgpu_ip_block_hw_fini() and then call it twice.

Sure. Noted.

> Regards,
> Christian.
>
> >           }
> >             adev->ip_blocks[i].status.hw = false;