| Message ID | 091915bb1cbec13b566d129f85ae229fcb92e2e4.1619191723.git.mchehab+huawei@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/78] media: atmel: properly get pm_runtime | expand |
Em Sun, 25 Apr 2021 22:57:25 +0200 Sylwester Nawrocki <snawrocki@kernel.org> 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 <mchehab+huawei@kernel.org> > > --- > > 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 <snawrocki@kernel.org> 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 <mchehab+huawei@kernel.org> >>> 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: >>
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; } }
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@kernel.org> --- 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(-)