| Message ID | 20240123095642.97303-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small runtime PM API changes | expand |
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.
diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst index 65b86e487afe..da99379071a4 100644 --- a/Documentation/power/runtime_pm.rst +++ b/Documentation/power/runtime_pm.rst @@ -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);` diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c index 0af8864cb3b5..c6d93c7a1c58 100644 --- a/drivers/accel/ivpu/ivpu_pm.c +++ b/drivers/accel/ivpu/ivpu_pm.c @@ -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; diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 05793c9fbb84..83e0cd2a19e7 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -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. diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 860b51b56a92..b5f8abd2a22b 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) return 0; } diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c index b429c2876a76..dd110058bf74 100644 --- a/drivers/gpu/drm/xe/xe_pm.c +++ b/drivers/gpu/drm/xe/xe_pm.c @@ -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) diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c index e21287d50c15..e1ae0f9fad43 100644 --- a/drivers/media/i2c/ccs/ccs-core.c +++ b/drivers/media/i2c/ccs/ccs-core.c @@ -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; diff --git a/drivers/media/i2c/ov64a40.c b/drivers/media/i2c/ov64a40.c index 4fba4c2cb064..541bf74581d2 100644 --- a/drivers/media/i2c/ov64a40.c +++ b/drivers/media/i2c/ov64a40.c @@ -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; diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c index 2806887514dc..19bd923a7315 100644 --- a/drivers/media/i2c/thp7312.c +++ b/drivers/media/i2c/thp7312.c @@ -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) { diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c index 5620dc271fac..cbf3d4761ce3 100644 --- a/drivers/net/ipa/ipa_smp2p.c +++ b/drivers/net/ipa/ipa_smp2p.c @@ -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); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d8f11a078924..f8293ae71389 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -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; diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 7c9b35448563..a212b3008ade 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -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; } diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index 7f7b67fe1b65..068c16e52dff 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -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)