[PATCH 1/2] drm/amdgpu: correct the amdgpu runtime dereference usage count

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Deucher, Alexander <Alexander.Deucher at amd.com>
> Sent: Friday, November 10, 2023 5:46 AM
> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: RE: [PATCH 1/2] drm/amdgpu: correct the amdgpu runtime
> dereference usage count
>
> [Public]
>
> > -----Original Message-----
> > From: Liang, Prike <Prike.Liang at amd.com>
> > Sent: Thursday, November 9, 2023 2:37 AM
> > To: amd-gfx at lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Liang, Prike
> > <Prike.Liang at amd.com>
> > Subject: [PATCH 1/2] drm/amdgpu: correct the amdgpu runtime
> > dereference usage count
> >
> > Fix the amdgpu runpm dereference usage count.
> >
> > Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > index a53f436fa9f1..f6bbbbe5d9f7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > @@ -1992,7 +1992,7 @@ static int amdgpu_debugfs_sclk_set(void *data,
> > u64 val)
> >
> >       ret = amdgpu_dpm_set_soft_freq_range(adev, PP_SCLK,
> > (uint32_t)val, (uint32_t)val);
> >       if (ret)
> > -             ret = -EINVAL;
> > +             goto out;
>
> I think this hunk can be dropped.  It doesn't really change anything.  Or you
> could just drop the whole ret check since we just return ret at the end
> anyway.  Not sure if changing the error code is important here or not.
>
[Prike] Thanks for pointing it out, revisit this part that seems ok for reassign the return value when the caller function can't return a proper error type.
I will keep this part as the unmodified since this has no problem for dereferencing the runpm usage.
> >
> >  out:
> >       pm_runtime_mark_last_busy(adev_to_drm(adev)->dev);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index 0cacd0b9f8be..ff1f42ae6d8e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -346,6 +346,7 @@ int amdgpu_display_crtc_set_config(struct
> > drm_mode_set *set,
> >       if (!active && adev->have_disp_power_ref) {
> >               pm_runtime_put_autosuspend(dev->dev);
> >               adev->have_disp_power_ref = false;
> > +             return ret;
> >       }
>
> I think it would be cleaner to just drop the runtime_put above and update
> the comment.  We'll just fall through to the end of the function.
>
> Alex
>
[Prike]  Do you mean something like as the following? I will submit a new patch for this.

-       /* if we have no active crtcs, then drop the power ref
-        * we got before
+       /* if we have no active crtcs, then go to
+        * drop the power ref we got before
         */
        if (!active && adev->have_disp_power_ref) {
-               pm_runtime_put_autosuspend(dev->dev);
                adev->have_disp_power_ref = false;
+               goto out;
        }
> >
> >  out:
> > --
> > 2.34.1
>