[v4,1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
Commit Message
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().
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Alex Elder <elder@linaro.org> # drivers/net/ipa/ipa_smp2p.c
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Takashi Iwai <tiwai@suse.de> # sound/
Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> # drivers/accel/ivpu/
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> # drivers/gpu/drm/i915/
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
Documentation/power/runtime_pm.rst | 5 ++--
drivers/accel/ivpu/ivpu_pm.c | 2 +-
drivers/base/power/runtime.c | 6 ++---
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c | 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 32 +++++++++++++++++++++----
sound/hda/hdac_device.c | 2 +-
12 files changed, 41 insertions(+), 20 deletions(-)
Comments
On Tue, Jan 23, 2024 at 11:56:42AM +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().
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Alex Elder <elder@linaro.org> # drivers/net/ipa/ipa_smp2p.c
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Takashi Iwai <tiwai@suse.de> # sound/
> Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> # drivers/accel/ivpu/
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> # drivers/gpu/drm/i915/
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci/
- Previous PM history uses "PM: " in the subject lines (not "pm: ").
- I don't know whether it's feasible, but it would be nice if the
intel_pm_runtime_pm.c rework could be done in one shot instead of
being split between patches 1/3 and 2/3.
Maybe it could be a preliminary patch that uses the existing
if_active/if_in_use interfaces, followed by the trivial if_active
updates in this patch. I think that would make the history easier
to read than having the transitory pm_runtime_get_conditional() in
the middle.
- Similarly, it would be nice if pm_runtime_get_conditional() never
had to be published in pm_runtime.h, instead of being temporarily
added there by this patch and then immediately made private by 2/3.
Maybe that's not practical, I dunno.
Bjorn
Hi Bjorn,
Thanks for the review.
On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 23, 2024 at 11:56:42AM +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().
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Alex Elder <elder@linaro.org> # drivers/net/ipa/ipa_smp2p.c
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Acked-by: Takashi Iwai <tiwai@suse.de> # sound/
> > Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> # drivers/accel/ivpu/
> > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> # drivers/gpu/drm/i915/
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci/
>
> - Previous PM history uses "PM: " in the subject lines (not "pm: ").
Oops. I'm not sure why I used lower case. (Maybe I've written too many
times "media:" prefix to the subject?) I'll fix this in v5.
>
> - I don't know whether it's feasible, but it would be nice if the
> intel_pm_runtime_pm.c rework could be done in one shot instead of
> being split between patches 1/3 and 2/3.
>
> Maybe it could be a preliminary patch that uses the existing
> if_active/if_in_use interfaces, followed by the trivial if_active
> updates in this patch. I think that would make the history easier
> to read than having the transitory pm_runtime_get_conditional() in
> the middle.
I think I'd merge the two patches. The second patch is fairly small, after
all, and both deal with largely the same code.
>
> - Similarly, it would be nice if pm_runtime_get_conditional() never
> had to be published in pm_runtime.h, instead of being temporarily
> added there by this patch and then immediately made private by 2/3.
> Maybe that's not practical, I dunno.
On Tue, Jan 23, 2024 at 08:44:04PM +0000, Sakari Ailus wrote:
> On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote:
> ...
> > - I don't know whether it's feasible, but it would be nice if the
> > intel_pm_runtime_pm.c rework could be done in one shot instead of
> > being split between patches 1/3 and 2/3.
> >
> > Maybe it could be a preliminary patch that uses the existing
> > if_active/if_in_use interfaces, followed by the trivial if_active
> > updates in this patch. I think that would make the history easier
> > to read than having the transitory pm_runtime_get_conditional() in
> > the middle.
>
> I think I'd merge the two patches. The second patch is fairly small, after
> all, and both deal with largely the same code.
I'm not sure which two patches you mean, but the fact that two patches
deal with largely the same code is not necessarily an argument for
merging them. From a reviewing perspective, it's nice if a patch like
1/3, where it's largely mechanical and easy to review, is separated
from patches that make more substantive changes.
That's why I think it'd be nice if the "interesting"
intel_pm_runtime_pm.c changes were all in the same patch, and ideally,
if that patch *only* touched intel_pm_runtime_pm.c.
Bjorn
On Tue, Jan 23, 2024 at 03:48:01PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 23, 2024 at 08:44:04PM +0000, Sakari Ailus wrote:
> > On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote:
> > ...
>
> > > - I don't know whether it's feasible, but it would be nice if the
> > > intel_pm_runtime_pm.c rework could be done in one shot instead of
> > > being split between patches 1/3 and 2/3.
> > >
> > > Maybe it could be a preliminary patch that uses the existing
> > > if_active/if_in_use interfaces, followed by the trivial if_active
> > > updates in this patch. I think that would make the history easier
> > > to read than having the transitory pm_runtime_get_conditional() in
> > > the middle.
> >
> > I think I'd merge the two patches. The second patch is fairly small, after
> > all, and both deal with largely the same code.
>
> I'm not sure which two patches you mean, but the fact that two patches
> deal with largely the same code is not necessarily an argument for
> merging them. From a reviewing perspective, it's nice if a patch like
Patches 1 and 2. The third patch introduces a new Runtime PM API function.
> 1/3, where it's largely mechanical and easy to review, is separated
> from patches that make more substantive changes.
>
> That's why I think it'd be nice if the "interesting"
> intel_pm_runtime_pm.c changes were all in the same patch, and ideally,
> if that patch *only* touched intel_pm_runtime_pm.c.
I don't think squashing the second patch to the first really changes this
meaningfully: the i915 driver simply needs both
pm_runtime_get_if_{active,in_use}, and this is what the patch does to other
drivers already. Making the pm_runtime_get_conditional static would also
fit for the first patch if the desire is to not to introduce it at all.
@@ -396,10 +396,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
nonzero, increment the counter and return 1; otherwise return 0 without
changing the counter
- `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
+ `int pm_runtime_get_if_active(struct device *dev);`
- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
- runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
- or the device's usage_count is non-zero, increment the counter and
+ runtime PM status is RPM_ACTIVE, increment the counter and
return 1; otherwise return 0 without changing the counter
`void pm_runtime_put_noidle(struct device *dev);`
@@ -292,7 +292,7 @@ int ivpu_rpm_get_if_active(struct ivpu_device *vdev)
{
int ret;
- ret = pm_runtime_get_if_active(vdev->drm.dev, false);
+ ret = pm_runtime_get_if_in_use(vdev->drm.dev);
drm_WARN_ON(&vdev->drm, ret < 0);
return ret;
@@ -1176,7 +1176,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
/**
- * pm_runtime_get_if_active - Conditionally bump up device usage counter.
+ * pm_runtime_get_conditional - Conditionally bump up device usage counter.
* @dev: Device to handle.
* @ign_usage_count: Whether or not to look at the current usage counter value.
*
@@ -1197,7 +1197,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
*/
-int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
+int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1218,7 +1218,7 @@ int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
return retval;
}
-EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+EXPORT_SYMBOL_GPL(pm_runtime_get_conditional);
/**
* __pm_runtime_set_status - Set runtime PM status of a device.
@@ -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)
return 0;
}
@@ -330,7 +330,7 @@ int xe_pm_runtime_put(struct xe_device *xe)
int xe_pm_runtime_get_if_active(struct xe_device *xe)
{
- return pm_runtime_get_if_active(xe->drm.dev, true);
+ return pm_runtime_get_if_active(xe->drm.dev);
}
void xe_pm_assert_unbounded_bridge(struct xe_device *xe)
@@ -674,7 +674,7 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_status = pm_runtime_get_if_active(&client->dev, true);
+ pm_status = pm_runtime_get_if_active(&client->dev);
if (!pm_status)
return 0;
@@ -3287,7 +3287,7 @@ static int ov64a40_set_ctrl(struct v4l2_ctrl *ctrl)
exp_max, 1, exp_val);
}
- pm_status = pm_runtime_get_if_active(ov64a40->dev, true);
+ pm_status = pm_runtime_get_if_active(ov64a40->dev);
if (!pm_status)
return 0;
@@ -1052,7 +1052,7 @@ static int thp7312_s_ctrl(struct v4l2_ctrl *ctrl)
if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
return -EINVAL;
- if (!pm_runtime_get_if_active(thp7312->dev, true))
+ if (!pm_runtime_get_if_active(thp7312->dev))
return 0;
switch (ctrl->id) {
@@ -92,7 +92,7 @@ static void ipa_smp2p_notify(struct ipa_smp2p *smp2p)
return;
dev = &smp2p->ipa->pdev->dev;
- smp2p->power_on = pm_runtime_get_if_active(dev, true) > 0;
+ smp2p->power_on = pm_runtime_get_if_active(dev) > 0;
/* Signal whether the IPA power is enabled */
mask = BIT(smp2p->enabled_bit);
@@ -2510,7 +2510,7 @@ static void pci_pme_list_scan(struct work_struct *work)
* If the device is in a low power state it
* should not be polled either.
*/
- pm_status = pm_runtime_get_if_active(dev, true);
+ pm_status = pm_runtime_get_if_active(dev);
if (!pm_status)
continue;
@@ -72,7 +72,8 @@ extern int pm_runtime_force_resume(struct device *dev);
extern int __pm_runtime_idle(struct device *dev, int rpmflags);
extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
extern int __pm_runtime_resume(struct device *dev, int rpmflags);
-extern int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);
+extern int pm_runtime_get_conditional(struct device *dev,
+ bool ign_usage_count);
extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
extern int pm_runtime_barrier(struct device *dev);
@@ -94,16 +95,33 @@ extern void pm_runtime_release_supplier(struct device_link *link);
extern int devm_pm_runtime_enable(struct device *dev);
+/**
+ * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
+ * in active state
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
+ * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
+ * device.
+ */
+static inline int pm_runtime_get_if_active(struct device *dev)
+{
+ return pm_runtime_get_conditional(dev, true);
+}
+
/**
* pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
* @dev: Target device.
*
* Increment the runtime PM usage counter of @dev if its runtime PM status is
- * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
+ * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
+ * it returns 1. If the device is in a different state or its usage_count is 0,
+ * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device.
*/
static inline int pm_runtime_get_if_in_use(struct device *dev)
{
- return pm_runtime_get_if_active(dev, false);
+ return pm_runtime_get_conditional(dev, false);
}
/**
@@ -275,8 +293,12 @@ static inline int pm_runtime_get_if_in_use(struct device *dev)
{
return -EINVAL;
}
-static inline int pm_runtime_get_if_active(struct device *dev,
- bool ign_usage_count)
+static inline int pm_runtime_get_if_active(struct device *dev)
+{
+ return -EINVAL;
+}
+static inline int pm_runtime_get_conditional(struct device *dev,
+ bool ign_usage_count)
{
return -EINVAL;
}
@@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm);
int snd_hdac_keep_power_up(struct hdac_device *codec)
{
if (!atomic_inc_not_zero(&codec->in_pm)) {
- int ret = pm_runtime_get_if_active(&codec->dev, true);
+ int ret = pm_runtime_get_if_active(&codec->dev);
if (!ret)
return -1;
if (ret < 0)