Re: [PATCH v4 1/3] PM: runtime: Add auto-cleanup macros for "resume and get" operations

[<dan.j.williams@intel.com>]

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is generally useful to be able to automatically drop a device's
> runtime PM usage counter incremented by runtime PM operations that
> resume a device and bump up its usage counter [1].
> 
> To that end, add guard definition macros allowing pm_runtime_put()
> and pm_runtime_put_autosuspend() to be used for the auto-cleanup in
> those cases.
> 
> Simply put, a piece of code like below:
> 
> 	pm_runtime_get_sync(dev);
> 	.....
> 	pm_runtime_put(dev);
> 	return 0;
> 
> can be transformed with guard() like:
> 
> 	guard(pm_runtime_active)(dev);
> 	.....
> 	return 0;
> 
> (see the pm_runtime_put() call is gone).
> 
> However, it is better to do proper error handling in the majority of
> cases, so doing something like this instead of the above is recommended:
> 
> 	ACQUIRE(pm_runtime_active_try, pm)(dev);
> 	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> 		return -ENXIO;
> 	.....
> 	return 0;
> 
> In all of the cases in which runtime PM is known to be enabled for the
> given device or the device can be regarded as operational (and so it can
> be accessed) with runtime PM disabled, a piece of code like:
> 
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	pm_runtime_put(dev);
> 	return 0;
> 
> can be changed as follows:
> 
> 	ACQUIRE(pm_runtime_active_try, pm)(dev);
> 	ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	return 0;
> 
> (again, see the pm_runtime_put() call is gone).
> 
> Still, if the device cannot be accessed unless runtime PM has been
> enabled for it, the CLASS(pm_runtime_get_active_enabled) variant

Leftover from CLASS() approach?

s/CLASS(pm_runtime_get_active_enabled)/ACQUIRE(pm_runtime_active_try_enabled)/

> needs to be used, that is (in the context of the example above):
> 
> 	ACQUIRE(pm_runtime_active_try_enabled, pm)(dev);
> 	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	return 0;
> 
> When the original code calls pm_runtime_put_autosuspend(), use one
> of the "auto" guard variants, pm_runtime_active_auto/_try/_enabled,
> so for example, a piece of code like:
> 
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	pm_runtime_put_autosuspend(dev);
> 	return 0;
> 
> will become:
> 
> 	ACQUIRE(pm_runtime_active_auto_try_enabled, pm)(dev);
> 	ret = ACQUIRE_ERR(pm_runtime_active_auto_try_enabled, &pm);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	return 0;
> 
> Note that the cases in which the return value of pm_runtime_get_sync()
> is checked can also be handled with the help of the new class macros.

s/class/guard/

> For example, a piece of code like:
> 
> 	ret = pm_runtime_get_sync(dev);
> 	if (ret < 0) {
> 		pm_runtime_put(dev);
> 		return ret;
> 	}
> 	.....
> 	pm_runtime_put(dev);
> 	return 0;
> 
> can be rewritten as:
> 
> 	ACQUIRE(pm_runtime_active_auto_try_enabled, pm)(dev);
> 	ret = ACQUIRE_ERR(pm_runtime_active_auto_try_enabled, &pm);
> 	if (ret < 0)
> 		return ret;
> 	.....
> 	return 0;

I like that this appears to unify the pm_runtime_resume_and_get() and
pm_runtime_get_sync() usages into common pattern.

> or pm_runtime_get_active_try can be used if transparent handling of
> disabled runtime PM is desirable.

Do you think the above should go in Documentation too?

Either way, for the usage of ACQUIRE(), looks good to me.

Acked-by: Dan Williams <dan.j.williams@intel.com>