Re: [PATCH] drm/xe/debugfs: Add missing xe_pm_runtime_put in wedge_mode_set

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Feb 14, 2025 at 02:12:58AM +0000, Lin, Shuicheng wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Thursday, February 13, 2025 3:23 PM
> > To: Lin, Shuicheng <shuicheng.lin@intel.com>
> > Cc: intel-xe@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/xe/debugfs: Add missing xe_pm_runtime_put in
> > wedge_mode_set
> > 
> > On Thu, Feb 13, 2025 at 11:03:22PM +0000, Shuicheng Lin wrote:
> > > xe_pm_runtime_put is missed in the failure path.
> > >
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_debugfs.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_debugfs.c
> > > b/drivers/gpu/drm/xe/xe_debugfs.c index 56cb3788e752..761e00cb6437
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_debugfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> > > @@ -176,6 +176,7 @@ static ssize_t wedged_mode_set(struct file *f, const
> > char __user *ubuf,
> > >  		ret = xe_guc_ads_scheduler_policy_toggle_reset(&gt-
> > >uc.guc.ads);
> > >  		if (ret) {
> > >  			xe_gt_err(gt, "Failed to update GuC ADS scheduler
> > policy. GuC may
> > > still cause engine reset even with wedged_mode=2\n");
> > > +			xe_pm_runtime_put(xe);
> > 
> > to ensure a git-grep alignment in the balance we try to use goto in cases like
> > this...
> > 
> > so it would be something like
> > 
> > ssize_t ret = size;
> > 
> > - return -EIO;
> > + ret = -EIO;
> > + goto out;
> > 
> > 
> > +out
> > xe_pm_runtime_put(xe);
> > 
> > - return size;
> > + return ret;
> 
> The ret value is changed to 0 with " ret = kstrtouint_from_user(ubuf, size, 0, &wedged_mode); ".
> So I have to set "ret=size;" before the "for_each_gt" loop.

Well, the kstrtouint_from_user is the actual size we want to return
anyway, so you shouldn't need to reset the ret.

if there was an error there it would had returned.

> It makes me prefer to go with this one. What is your idea about it?

Anyway, I respect your choice there. I just pushed the patch to drm-xe-next.
Thanks for the fix.

> Thanks.
> 
> > 
> > But well, the code itself is right and better to protect.
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > just let me know if you are taking the goto approach or if I should get this one
> > merged.
> > 
> > Thanks,
> > Rodrigo.
> > 
> > >  			return -EIO;
> > >  		}
> > >  	}
> > > --
> > > 2.25.1
> > >