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 <[email protected]>
---
drivers/media/platform/exynos4-is/fimc-capture.c | 6 ++----
drivers/media/platform/exynos4-is/fimc-is.c | 3 ++-
drivers/media/platform/exynos4-is/fimc-isp-video.c | 3 +--
drivers/media/platform/exynos4-is/fimc-isp.c | 7 +++----
drivers/media/platform/exynos4-is/fimc-lite.c | 5 +++--
drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
drivers/media/platform/exynos4-is/media-dev.c | 8 +++-----
drivers/media/platform/exynos4-is/mipi-csis.c | 5 ++---
8 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 13c838d3f947..0da36443173c 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -478,11 +478,9 @@ static int fimc_capture_open(struct file *file)
goto unlock;
set_bit(ST_CAPT_BUSY, &fimc->state);
- ret = pm_runtime_get_sync(&fimc->pdev->dev);
- if (ret < 0) {
- pm_runtime_put_sync(&fimc->pdev->dev);
+ ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
+ if (ret < 0)
goto unlock;
- }
ret = v4l2_fh_open(file);
if (ret) {
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index 972d9601d236..bca35866cc74 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -828,7 +828,7 @@ static int fimc_is_probe(struct platform_device *pdev)
goto err_irq;
}
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
goto err_pm;
@@ -862,6 +862,7 @@ static int fimc_is_probe(struct platform_device *pdev)
fimc_is_unregister_subdevs(is);
err_pm:
pm_runtime_put_noidle(dev);
+err_suspend:
if (!pm_runtime_enabled(dev))
fimc_is_runtime_suspend(dev);
err_irq:
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c
index 612b9872afc8..8d9dc597deaa 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
@@ -275,7 +275,7 @@ static int isp_video_open(struct file *file)
if (ret < 0)
goto unlock;
- ret = pm_runtime_get_sync(&isp->pdev->dev);
+ ret = pm_runtime_resume_and_get(&isp->pdev->dev);
if (ret < 0)
goto rel_fh;
@@ -293,7 +293,6 @@ static int isp_video_open(struct file *file)
if (!ret)
goto unlock;
rel_fh:
- pm_runtime_put_noidle(&isp->pdev->dev);
v4l2_fh_release(file);
unlock:
mutex_unlock(&isp->video_lock);
diff --git a/drivers/media/platform/exynos4-is/fimc-isp.c b/drivers/media/platform/exynos4-is/fimc-isp.c
index a77c49b18511..74b49d30901e 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp.c
@@ -304,11 +304,10 @@ static int fimc_isp_subdev_s_power(struct v4l2_subdev *sd, int on)
pr_debug("on: %d\n", on);
if (on) {
- ret = pm_runtime_get_sync(&is->pdev->dev);
- if (ret < 0) {
- pm_runtime_put(&is->pdev->dev);
+ ret = pm_runtime_resume_and_get(&is->pdev->dev);
+ if (ret < 0)
return ret;
- }
+
set_bit(IS_ST_PWR_ON, &is->state);
ret = fimc_is_start_firmware(is);
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index fe20af3a7178..4d8b18078ff3 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -469,9 +469,9 @@ static int fimc_lite_open(struct file *file)
}
set_bit(ST_FLITE_IN_USE, &fimc->state);
- ret = pm_runtime_get_sync(&fimc->pdev->dev);
+ ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
if (ret < 0)
- goto err_pm;
+ goto err_in_use;
ret = v4l2_fh_open(file);
if (ret < 0)
@@ -499,6 +499,7 @@ static int fimc_lite_open(struct file *file)
v4l2_fh_release(file);
err_pm:
pm_runtime_put_sync(&fimc->pdev->dev);
+err_in_use:
clear_bit(ST_FLITE_IN_USE, &fimc->state);
unlock:
mutex_unlock(&fimc->lock);
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index c9704a147e5c..7c1eb05c508f 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -75,7 +75,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
struct fimc_ctx *ctx = q->drv_priv;
int ret;
- ret = pm_runtime_get_sync(&ctx->fimc_dev->pdev->dev);
+ ret = pm_runtime_resume_and_get(&ctx->fimc_dev->pdev->dev);
return ret > 0 ? 0 : ret;
}
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 13d192ba4aa6..9346d44a06c2 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -512,11 +512,9 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
if (!fmd->pmf)
return -ENXIO;
- ret = pm_runtime_get_sync(fmd->pmf);
- if (ret < 0) {
- pm_runtime_put(fmd->pmf);
+ ret = pm_runtime_resume_and_get(fmd->pmf);
+ if (ret < 0)
return ret;
- }
fmd->num_sensors = 0;
@@ -1291,7 +1289,7 @@ static int cam_clk_prepare(struct clk_hw *hw)
if (camclk->fmd->pmf == NULL)
return -ENODEV;
- ret = pm_runtime_get_sync(camclk->fmd->pmf);
+ ret = pm_runtime_resume_and_get(camclk->fmd->pmf);
return ret < 0 ? ret : 0;
}
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 1aac167abb17..a0218237d66b 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -494,7 +494,7 @@ static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
struct device *dev = &state->pdev->dev;
if (on)
- return pm_runtime_get_sync(dev);
+ return pm_runtime_resume_and_get(dev);
return pm_runtime_put_sync(dev);
}
@@ -509,9 +509,8 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
if (enable) {
s5pcsis_clear_counters(state);
- ret = pm_runtime_get_sync(&state->pdev->dev);
+ ret = pm_runtime_resume_and_get(&state->pdev->dev);
if (ret && ret != 1) {
- pm_runtime_put_noidle(&state->pdev->dev);
return ret;
}
}
--
2.30.2
On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
> 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 <[email protected]>
> ---
> drivers/media/platform/exynos4-is/fimc-capture.c | 6 ++----
> drivers/media/platform/exynos4-is/fimc-is.c | 3 ++-
> drivers/media/platform/exynos4-is/fimc-isp-video.c | 3 +--
> drivers/media/platform/exynos4-is/fimc-isp.c | 7 +++----
> drivers/media/platform/exynos4-is/fimc-lite.c | 5 +++--
> drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
> drivers/media/platform/exynos4-is/media-dev.c | 8 +++-----
> drivers/media/platform/exynos4-is/mipi-csis.c | 5 ++---
> 8 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
> index 13c838d3f947..0da36443173c 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -478,11 +478,9 @@ static int fimc_capture_open(struct file *file)
> goto unlock;
>
> set_bit(ST_CAPT_BUSY, &fimc->state);
> - ret = pm_runtime_get_sync(&fimc->pdev->dev);
> - if (ret < 0) {
> - pm_runtime_put_sync(&fimc->pdev->dev);
> + ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
> + if (ret < 0)
> goto unlock;
> - }
>
> ret = v4l2_fh_open(file);
> if (ret) {
> diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
> index 972d9601d236..bca35866cc74 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is.c
> @@ -828,7 +828,7 @@ static int fimc_is_probe(struct platform_device *pdev)
> goto err_irq;
> }
>
> - ret = pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> goto err_pm;
It seems you intended to use err_suspend label here. We don't need
a new label though, instead of err_pm we can jump to err_irq when
pm_runtime_resume_and_get() fails. Note that when runtime PM is
disabled pm_runtime_resume_and_get() always returns 0.
> @@ -862,6 +862,7 @@ static int fimc_is_probe(struct platform_device *pdev)
> fimc_is_unregister_subdevs(is);
> err_pm:
> pm_runtime_put_noidle(dev);
> +err_suspend:
> if (!pm_runtime_enabled(dev))
> fimc_is_runtime_suspend(dev);
> err_irq:
> diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
> index 1aac167abb17..a0218237d66b 100644
> --- a/drivers/media/platform/exynos4-is/mipi-csis.c
> +++ b/drivers/media/platform/exynos4-is/mipi-csis.c
> @@ -494,7 +494,7 @@ static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
> struct device *dev = &state->pdev->dev;
>
> if (on)
> - return pm_runtime_get_sync(dev);
> + return pm_runtime_resume_and_get(dev);
>
> return pm_runtime_put_sync(dev);
> }
> @@ -509,9 +509,8 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
>
> if (enable) {
> s5pcsis_clear_counters(state);
> - ret = pm_runtime_get_sync(&state->pdev->dev);
> + ret = pm_runtime_resume_and_get(&state->pdev->dev);
> if (ret && ret != 1) {
> - pm_runtime_put_noidle(&state->pdev->dev);
> return ret;
> }
Braces could be dropped as well here.
> }
Thanks,
Sylwester
Em Sun, 25 Apr 2021 22:57:25 +0200
Sylwester Nawrocki <[email protected]> escreveu:
> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
> > 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 <[email protected]>
> > ---
> > drivers/media/platform/exynos4-is/fimc-capture.c | 6 ++----
> > drivers/media/platform/exynos4-is/fimc-is.c | 3 ++-
> > drivers/media/platform/exynos4-is/fimc-isp-video.c | 3 +--
> > drivers/media/platform/exynos4-is/fimc-isp.c | 7 +++----
> > drivers/media/platform/exynos4-is/fimc-lite.c | 5 +++--
> > drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
> > drivers/media/platform/exynos4-is/media-dev.c | 8 +++-----
> > drivers/media/platform/exynos4-is/mipi-csis.c | 5 ++---
> > 8 files changed, 17 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
> > index 13c838d3f947..0da36443173c 100644
> > --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> > +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> > @@ -478,11 +478,9 @@ static int fimc_capture_open(struct file *file)
> > goto unlock;
> >
> > set_bit(ST_CAPT_BUSY, &fimc->state);
> > - ret = pm_runtime_get_sync(&fimc->pdev->dev);
> > - if (ret < 0) {
> > - pm_runtime_put_sync(&fimc->pdev->dev);
> > + ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
> > + if (ret < 0)
> > goto unlock;
> > - }
> >
> > ret = v4l2_fh_open(file);
> > if (ret) {
> > diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
> > index 972d9601d236..bca35866cc74 100644
> > --- a/drivers/media/platform/exynos4-is/fimc-is.c
> > +++ b/drivers/media/platform/exynos4-is/fimc-is.c
> > @@ -828,7 +828,7 @@ static int fimc_is_probe(struct platform_device *pdev)
> > goto err_irq;
> > }
> >
> > - ret = pm_runtime_get_sync(dev);
> > + ret = pm_runtime_resume_and_get(dev);
> > if (ret < 0)
> > goto err_pm;
>
> It seems you intended to use err_suspend label here. We don't need
> a new label though, instead of err_pm we can jump to err_irq when
> pm_runtime_resume_and_get() fails.
Thanks! Will fix at the next version.
> Note that when runtime PM is
> disabled pm_runtime_resume_and_get() always returns 0.
Ok, but there are a couple of conditions at rpm_resume() function
at drivers/base/power/runtime.c (which is the code that actually
handles those PM macros) that could make it to return errors,
which are independent on the PM callbacks, like those:
if (dev->power.runtime_error)
retval = -EINVAL;
else if (dev->power.disable_depth > 0)
retval = -EACCES;
and more might be added as the PM core changes.
>
> > @@ -862,6 +862,7 @@ static int fimc_is_probe(struct platform_device *pdev)
> > fimc_is_unregister_subdevs(is);
> > err_pm:
> > pm_runtime_put_noidle(dev);
> > +err_suspend:
> > if (!pm_runtime_enabled(dev))
> > fimc_is_runtime_suspend(dev);
> > err_irq:
>
>
> > diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
> > index 1aac167abb17..a0218237d66b 100644
> > --- a/drivers/media/platform/exynos4-is/mipi-csis.c
> > +++ b/drivers/media/platform/exynos4-is/mipi-csis.c
> > @@ -494,7 +494,7 @@ static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
> > struct device *dev = &state->pdev->dev;
> >
> > if (on)
> > - return pm_runtime_get_sync(dev);
> > + return pm_runtime_resume_and_get(dev);
> >
> > return pm_runtime_put_sync(dev);
> > }
> > @@ -509,9 +509,8 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
> >
> > if (enable) {
> > s5pcsis_clear_counters(state);
> > - ret = pm_runtime_get_sync(&state->pdev->dev);
> > + ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > if (ret && ret != 1) {
> > - pm_runtime_put_noidle(&state->pdev->dev);
> > return ret;
> > }
>
> Braces could be dropped as well here.
OK.
>
> > }
>
>
> Thanks,
> Sylwester
>
Thanks,
Mauro
On 26.04.2021 15:12, Mauro Carvalho Chehab wrote:
> Em Sun, 25 Apr 2021 22:57:25 +0200
> Sylwester Nawrocki <[email protected]> escreveu:
>
>> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
>>> 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 <[email protected]>
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
>>> index 972d9601d236..bca35866cc74 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-is.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-is.c
>>> @@ -828,7 +828,7 @@ static int fimc_is_probe(struct platform_device *pdev)
>>> goto err_irq;
>>> }
>>>
>>> - ret = pm_runtime_get_sync(dev);
>>> + ret = pm_runtime_resume_and_get(dev);
>>> if (ret < 0)
>>> goto err_pm;
>>
>> It seems you intended to use err_suspend label here. We don't need
>> a new label though, instead of err_pm we can jump to err_irq when
>> pm_runtime_resume_and_get() fails.
>
> Thanks! Will fix at the next version.
>
>> Note that when runtime PM is
>> disabled pm_runtime_resume_and_get() always returns 0.
>
> Ok, but there are a couple of conditions at rpm_resume() function
> at drivers/base/power/runtime.c (which is the code that actually
> handles those PM macros) that could make it to return errors,
> which are independent on the PM callbacks, like those:
>
> if (dev->power.runtime_error)
> retval = -EINVAL;
> else if (dev->power.disable_depth > 0)
> retval = -EACCES;
>
> and more might be added as the PM core changes.
Right, I looked only at !CONFIG_PM case, this is what the "if (!pm_runtime_enabled(dev))"
test and explicit fimc_is_runtime_{resume,suspend} calls were originally for.
Agreed, better not to rely too much on internal implementation as there is
no specific guarantees about return value at the API documentation.
Regards,
Sylwester
>>> @@ -862,6 +862,7 @@ static int fimc_is_probe(struct platform_device *pdev)
>>> fimc_is_unregister_subdevs(is);
>>> err_pm:
>>> pm_runtime_put_noidle(dev);
>>> +err_suspend:
>>> if (!pm_runtime_enabled(dev))
>>> fimc_is_runtime_suspend(dev);
>>> err_irq:
>>