On Mon, Nov 13, 2023 at 10:02 PM Liang, Prike <Prike.Liang@xxxxxxx> wrote:
>
> [Public]
>
> > -----Original Message-----
> > From: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> > Sent: Friday, November 10, 2023 5:46 AM
> > To: Liang, Prike <Prike.Liang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: RE: [PATCH 1/2] drm/amdgpu: correct the amdgpu runtime
> > dereference usage count
> >
> > [Public]
> >
> > > -----Original Message-----
> > > From: Liang, Prike <Prike.Liang@xxxxxxx>
> > > Sent: Thursday, November 9, 2023 2:37 AM
> > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Liang, Prike
> > > <Prike.Liang@xxxxxxx>
> > > 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@xxxxxxx>
> > > ---
> > > 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
> >
>
