Re: [PATCH 22/51] iommu/arm-smmu: Switch to __pm_runtime_put_autosuspend()

[Will Deacon <will@kernel.org>]

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