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