Re: [PATCH 1/7] drm/xe: Fix xe_pm_runtime_get_if_active return

[Francois Dugast <francois.dugast@intel.com>]

On Mon, May 06, 2024 at 01:47:56PM +0200, Thomas Hellström wrote:
> On Fri, 2024-05-03 at 15:13 -0400, Rodrigo Vivi wrote:
> > Current callers of this function are already taking the result
> > to a boolean and using in an if. It might be a problem because
> > current function might return negative error codes on failure,
> > without increasing the reference counter.
> > 
> > In this scenario we could end up with extra 'put' call ending
> > in unbalanced scenarios.
> > 
> > Let's fix it, while aligning with the current xe_pm_get_if_in_use
> > style.
> > 
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pm.c | 8 ++++----
> >  drivers/gpu/drm/xe/xe_pm.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index c1831106ea4b..eca977d1e1cb 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -505,12 +505,12 @@ int xe_pm_runtime_get_ioctl(struct xe_device
> > *xe)
> >   * xe_pm_runtime_get_if_active - Get a runtime_pm reference if
> > device active
> >   * @xe: xe device instance
> >   *
> > - * Returns: Any number greater than or equal to 0 for success,
> > negative error
> > - * code otherwise.
> > + * Returns: True if device is awake (regardless the previous number
> > of references)
> Nit: Athough both "Returns:" and "Return:" work, the latter seems to be
> the preferred.

Hi Thomas,

This is indeed what grep shows for Xe (for DRM it is less obvious). The
doc mentions "Returns" in Documentation/gpu/introduction.rst, should we
update it?

Francois

> 
> Otherwise LGTM.
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
> 
> 
> > + * and a new reference was taken, false otherwise.
> >   */
> > -int xe_pm_runtime_get_if_active(struct xe_device *xe)
> > +bool xe_pm_runtime_get_if_active(struct xe_device *xe)
> >  {
> > -	return pm_runtime_get_if_active(xe->drm.dev);
> > +	return pm_runtime_get_if_active(xe->drm.dev) > 0;
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > index 18b0613fe57b..f694005db278 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.h
> > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > @@ -29,7 +29,7 @@ int xe_pm_runtime_resume(struct xe_device *xe);
> >  void xe_pm_runtime_get(struct xe_device *xe);
> >  int xe_pm_runtime_get_ioctl(struct xe_device *xe);
> >  void xe_pm_runtime_put(struct xe_device *xe);
> > -int xe_pm_runtime_get_if_active(struct xe_device *xe);
> > +bool xe_pm_runtime_get_if_active(struct xe_device *xe);
> >  bool xe_pm_runtime_get_if_in_use(struct xe_device *xe);
> >  void xe_pm_runtime_get_noresume(struct xe_device *xe);
> >  bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
>