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

[Alex Deucher]

On Mon, Nov 13, 2023 at 10:02 PM Liang, Prike <Prike.Liang at amd.com> wrote:
>
> [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.

Just drop the goto out.  E.g.

         if (!active && adev->have_disp_power_ref)
                adev->have_disp_power_ref = false;

Alex

>
> -       /* 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
> >
>