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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Francois,

On Mon, 2024-05-06 at 15:15 +0200, Francois Dugast wrote:
> 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?

I've tried to use https://docs.kernel.org/doc-guide/kernel-doc.html as
a reference, which uses "Return", so I've been trying to enforce that
for xe reviews.

/Thomas

> 
> 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);
> >