Re: [PATCH 17/78] staging: media: vde: use pm_runtime_resume_and_get()

[Johan Hovold]

On Tue, Apr 27, 2021 at 11:22:50AM +0200, Mauro Carvalho Chehab wrote:
> Hi Dmitry,
> 
> Em Sat, 24 Apr 2021 10:35:22 +0300
> Dmitry Osipenko <digetx@xxxxxxxxx> escreveu:
> 
> > 24.04.2021 09:44, Mauro Carvalho Chehab пишет:
> > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors.
> > > 
> > > Use the new API, in order to cleanup the error check logic.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  drivers/staging/media/tegra-vde/vde.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
> > > index 28845b5bafaf..8936f140a246 100644
> > > --- a/drivers/staging/media/tegra-vde/vde.c
> > > +++ b/drivers/staging/media/tegra-vde/vde.c
> > > @@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
> > >  	if (ret)
> > >  		goto release_dpb_frames;
> > >  
> > > -	ret = pm_runtime_get_sync(dev);
> > > +	ret = pm_runtime_resume_and_get(dev);
> > >  	if (ret < 0)
> > > -		goto put_runtime_pm;
> > > +		goto unlock;
> > >  
> > >  	/*
> > >  	 * We rely on the VDE registers reset value, otherwise VDE
> > > @@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
> > >  put_runtime_pm:
> > >  	pm_runtime_mark_last_busy(dev);
> > >  	pm_runtime_put_autosuspend(dev);
> > > +
> > > +unlock:
> > >  	mutex_unlock(&vde->lock);
> > >  
> > >  release_dpb_frames:
> > > @@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
> > >  	 * power-cycle it in order to put hardware into a predictable lower
> > >  	 * power state.
> > >  	 */
> > > -	pm_runtime_get_sync(dev);
> > > -	pm_runtime_put(dev);
> > > +	if (pm_runtime_resume_and_get(dev) >= 0)
> > > +		pm_runtime_put(dev);
> > >  
> > >  	return 0;
> > >  
> > > @@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
> > >  {
> > >  	struct tegra_vde *vde = platform_get_drvdata(pdev);
> > >  	struct device *dev = &pdev->dev;
> > > +	int ret;
> > >  
> > > -	pm_runtime_get_sync(dev);
> > > +	ret = pm_runtime_resume_and_get(dev);
> > >  	pm_runtime_dont_use_autosuspend(dev);
> > >  	pm_runtime_disable(dev);
> > >  
> > > @@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device *pdev)
> > >  	 * Balance RPM state, the VDE power domain is left ON and hardware
> > >  	 * is clock-gated. It's safe to reboot machine now.
> > >  	 */
> > > -	pm_runtime_put_noidle(dev);
> > > +	if (ret >= 0)
> > > +		pm_runtime_put_noidle(dev);
> > >  	clk_disable_unprepare(vde->clk);
> > >  
> > >  	misc_deregister(&vde->miscdev);
> > >   
> > 
> > Hello Mauro,
> > 
> > Thank you very much for the patch. It looks to me that the original
> > variant was a bit simpler, this patch adds more code lines without
> > changing the previous behaviour. Or am I missing something?

I agree, the above does not look like an improvement at all.

> While on several places the newer code is simpler, the end goal here is
> to replace all occurrences of pm_runtime_get_sync() from the media 
> subsystem, due to the number of problems we're having with this:
> 
> 1. despite its name, this is actually a PM runtime resume call,
>    but some developers didn't seem to realize that, as I got this
>    pattern on some drivers:
> 
>         pm_runtime_get_sync(&client->dev);
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
>         pm_runtime_put_noidle(&client->dev);
> 
>    It makes no sense to resume PM just to suspend it again ;-)

It very well may. You're resuming the device and leaving it a defined
power state before balancing the PM count, cleaning up and unbinding the
driver.

>    The name of the new variant is a lot clearer:
> 	pm_runtime_resume_and_get()

For people not used to the API perhaps.

> 2. Usual *_get() methods only increment their use count on success,
>    but pm_runtime_get_sync() increments it unconditionally. Due to
>    that, several drivers were mistakenly not calling
>    pm_runtime_put_noidle() when it fails;

As I mentioned elsewhere, all pm_runtime_get calls increment the usage
count so the API is consistent.
 
> 3. Consistency: we did similar changes subsystem wide with
>    for instance strlcpy() and strcpy() that got replaced by
>    strscpy(). Having all drivers using the same known-to-be-safe
>    methods is a good thing;

There's no know-to-be safe API. People will find ways to get this wrong
too.

And the old interface isn't going away from the kernel even if you
manage to not use it in media.

> 4. Prevent newer drivers to copy-and-paste a code that it would
>    be easier to break if they don't truly understand what's behind
>    the scenes.

Johan