Re: [PATCH 1/2] pm: runtime: Simplify pm_runtime_get_if_active() usage

Sakari Ailus
2024-01-09T19:09:04Z

Hi Rodrigo,

Thanks for the review.

On Tue, Jan 09, 2024 at 12:35:25PM -0500, Rodrigo Vivi wrote:
> On Tue, Jan 09, 2024 at 03:36:57PM +0200, Sakari Ailus wrote:
> > There are two ways to opportunistically increment a device's runtime PM
> > usage count, calling either pm_runtime_get_if_active() or
> > pm_runtime_get_if_in_use(). The former has an argument to tell whether to
> > ignore the usage count or not, and the latter simply calls the former with
> > ign_usage_count set to false. The other users that want to ignore the
> > usage_count will have to explitly set that argument to true which is a bit
> > cumbersome.
> >
> > To make this function more practical to use, remove the ign_usage_count
> > argument from the function. The main implementation is renamed as
> > __pm_runtime_get_conditional().
>
> Great idea! I recently found a bug in Xe caused by some confusion
> with the ign_usage_count var.
>
> > + * This function is not intended to be called by drivers, use
> > + * pm_runtime_get_if_active() or pm_runtime_get_if_in_use() instead.
> > */
>
> Well, it is not intended, but we already have an usage for that.
> So, shouldn't we avoid the '__' and just put as recommendation?
> Like we have around the get_sync telling that the preferred way is
> the resume_and_get for a cleaner code?

I'll do that in v2, it's indeed not an issue to call this directly in a
driver as you have ign_usage_count value available. Which is why the
comment says "intended". Feel free to propose a better wording.

pm_runtime_resume_and_get() isn't a universal replacement for
pm_runtime_get_sync() as pm_runtime_resume_and_get() won't return 1 if the
device was already active---this was considered a possible source of bugs
AFAIR. So there will be drivers using pm_runtime_get_sync().

>
> > -EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
> > +EXPORT_SYMBOL_GPL(__pm_runtime_get_conditional);
>
> Also the '__' in the exported and documented one doesn't look so nice.
> Well, I know that this would be the case even without any driver using
> because of the static inline anyway.
>
> >
> > /**
> > * __pm_runtime_set_status - Set runtime PM status of a device.
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 860b51b56a92..51cbca330bd5 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -246,7 +246,7 @@ static intel_wakeref_t __intel_runtime_pm_get_if_active(struct intel_runtime_pm
> > * function, since the power state is undefined. This applies
> > * atm to the late/early system suspend/resume handlers.
> > */
> > - if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0)
> > + if (__pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0)
>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks!

--
Regards,

Sakari Ailus