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

[Pranjal Shrivastava <praan@google.com>]

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