| Message ID | 20241004094123.113725-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | treewide: Switch to __pm_runtime_put_autosuspend() | expand |
On Fri, Oct 04, 2024 at 12:41:23PM +0300, Sakari Ailus wrote: > pm_runtime_put_autosuspend() will soon be changed to include a call to > pm_runtime_mark_last_busy(). This patch switches the current users to > __pm_runtime_put_autosuspend() which will continue to have the > functionality of old pm_runtime_put_autosuspend(). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index 8321962b3714..cad02d5dc6d2 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -79,7 +79,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > { > if (pm_runtime_enabled(smmu->dev)) > - pm_runtime_put_autosuspend(smmu->dev); > + __pm_runtime_put_autosuspend(smmu->dev); > } Seems like a straightforward change as a result of [1]. Although, I had a few things to discuss: 1. The `rpm_resume` in drivers/base/power/runtime.c seems to call `pm_runtime_mark_last_busy` in case the ->runtime_resume callback returned successfully. In such a case, why would we want to move `pm_runtime_mark_last_busy` within `pm_runtime_put_autosuspend` ? 2. In the arm-smmu driver, we seem to rely on the rpm_resume to call `pm_runtime_mark_last_busy` as a part of the ->runtime_resume callback. The only other case, where we might wanna `*mark_last_busy` is if we want the autosuspend timer to be re-started in case of a failed suspend. However, the `arm_smmu_runtime_suspend` doesn't return errno in any case hence, I don't see any other case where we'd benefit from using `mark_last_busy` in the arm-smmu driver. On the other hand, I don't see a problem with using it either :) Any thoughts Will/Rob/Robin? > > static void arm_smmu_rpm_use_autosuspend(struct arm_smmu_device *smmu) > -- > 2.39.5 > Apart from the above discussion, for this patch alone: Reviewed-by: Pranjal Shrivastava <praan@google.com> Thanks, Pranjal [1] https://lore.kernel.org/all/20240109133639.111210-1-sakari.ailus@linux.intel.com/
On Thu, Oct 10, 2024 at 03:33:11PM +0000, Pranjal Shrivastava wrote: > On Fri, Oct 04, 2024 at 12:41:23PM +0300, Sakari Ailus wrote: > > pm_runtime_put_autosuspend() will soon be changed to include a call to > > pm_runtime_mark_last_busy(). This patch switches the current users to > > __pm_runtime_put_autosuspend() which will continue to have the > > functionality of old pm_runtime_put_autosuspend(). > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > index 8321962b3714..cad02d5dc6d2 100644 > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > @@ -79,7 +79,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > > { > > if (pm_runtime_enabled(smmu->dev)) > > - pm_runtime_put_autosuspend(smmu->dev); > > + __pm_runtime_put_autosuspend(smmu->dev); > > } > > Seems like a straightforward change as a result of [1]. > Although, I had a few things to discuss: > > 1. The `rpm_resume` in drivers/base/power/runtime.c seems to call > `pm_runtime_mark_last_busy` in case the ->runtime_resume callback > returned successfully. In such a case, why would we want to move > `pm_runtime_mark_last_busy` within `pm_runtime_put_autosuspend` ? > > 2. In the arm-smmu driver, we seem to rely on the rpm_resume to call > `pm_runtime_mark_last_busy` as a part of the ->runtime_resume callback. > The only other case, where we might wanna `*mark_last_busy` is if we > want the autosuspend timer to be re-started in case of a failed suspend. > However, the `arm_smmu_runtime_suspend` doesn't return errno in any case > hence, I don't see any other case where we'd benefit from using > `mark_last_busy` in the arm-smmu driver. > > On the other hand, I don't see a problem with using it either :) > Any thoughts Will/Rob/Robin? To be honest, I think the current driver code is pretty weird. We're calling arm_smmu_rpm_use_autosuspend() during device attach and that does: pm_runtime_set_autosuspend_delay(smmu->dev, 20); pm_runtime_use_autosuspend(smmu->dev); whereas I would've expected these autosuspend parameters to be configured once during SMMU probe and then for attach to do something like: pm_runtime_mark_last_busy(); __pm_runtime_put_autosuspend(); So I think we should probably rework the code we have slightly, which will hopefully make this giant refactoring series a little more straightforward. Will
On Wed, Oct 23, 2024 at 05:48:36PM +0100, Will Deacon wrote: > On Thu, Oct 10, 2024 at 03:33:11PM +0000, Pranjal Shrivastava wrote: > > On Fri, Oct 04, 2024 at 12:41:23PM +0300, Sakari Ailus wrote: > > > pm_runtime_put_autosuspend() will soon be changed to include a call to > > > pm_runtime_mark_last_busy(). This patch switches the current users to > > > __pm_runtime_put_autosuspend() which will continue to have the > > > functionality of old pm_runtime_put_autosuspend(). > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > index 8321962b3714..cad02d5dc6d2 100644 > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > > @@ -79,7 +79,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > > > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > > > { > > > if (pm_runtime_enabled(smmu->dev)) > > > - pm_runtime_put_autosuspend(smmu->dev); > > > + __pm_runtime_put_autosuspend(smmu->dev); > > > } > > > > Seems like a straightforward change as a result of [1]. > > Although, I had a few things to discuss: > > > > 1. The `rpm_resume` in drivers/base/power/runtime.c seems to call > > `pm_runtime_mark_last_busy` in case the ->runtime_resume callback > > returned successfully. In such a case, why would we want to move > > `pm_runtime_mark_last_busy` within `pm_runtime_put_autosuspend` ? > > > > 2. In the arm-smmu driver, we seem to rely on the rpm_resume to call > > `pm_runtime_mark_last_busy` as a part of the ->runtime_resume callback. > > The only other case, where we might wanna `*mark_last_busy` is if we > > want the autosuspend timer to be re-started in case of a failed suspend. > > However, the `arm_smmu_runtime_suspend` doesn't return errno in any case > > hence, I don't see any other case where we'd benefit from using > > `mark_last_busy` in the arm-smmu driver. > > > > On the other hand, I don't see a problem with using it either :) > > Any thoughts Will/Rob/Robin? > > To be honest, I think the current driver code is pretty weird. We're > calling arm_smmu_rpm_use_autosuspend() during device attach and that > does: > > pm_runtime_set_autosuspend_delay(smmu->dev, 20); > pm_runtime_use_autosuspend(smmu->dev); > > whereas I would've expected these autosuspend parameters to be configured > once during SMMU probe and then for attach to do something like: > > pm_runtime_mark_last_busy(); > __pm_runtime_put_autosuspend(); > Ack. I had missed the `*set_autosuspend` call during attach, there's no need for that to happen with each attach. I agree that we should setup autosuspend delay once during the smmu probe and mark_last_busy on attach to retain the current behaviour. > So I think we should probably rework the code we have slightly, which > will hopefully make this giant refactoring series a little more > straightforward. +1. > > Will Thanks, Pranjal
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index 8321962b3714..cad02d5dc6d2 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -79,7 +79,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) { if (pm_runtime_enabled(smmu->dev)) - pm_runtime_put_autosuspend(smmu->dev); + __pm_runtime_put_autosuspend(smmu->dev); } static void arm_smmu_rpm_use_autosuspend(struct arm_smmu_device *smmu)
pm_runtime_put_autosuspend() will soon be changed to include a call to pm_runtime_mark_last_busy(). This patch switches the current users to __pm_runtime_put_autosuspend() which will continue to have the functionality of old pm_runtime_put_autosuspend(). Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)