During the review of the patches from unm.edu, one of the patterns
I noticed is the amount of patches trying to fix pm_runtime_get_sync()
calls.
After analyzing the feedback from version 1 of this series, I noticed
a few other weird behaviors at the PM runtime resume code. So, this
series start addressing some bugs and issues at the current code.
Then, it gets rid of pm_runtime_get_sync() at the media subsystem
(with 2 exceptions).
It should be noticed that
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added a new method to does a pm_runtime get, which increments
the usage count only on success.
The rationale of getting rid of pm_runtime_get_sync() is:
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 ;-)
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;
3. The name of the new variant is a lot clearer:
pm_runtime_resume_and_get()
As its same clearly says that this is a PM runtime resume function,
that also increments the usage counter on success;
4. 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;
5. 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.
This series replace places pm_runtime_get_sync(), by calling
pm_runtime_resume_and_get() instead.
This should help to avoid future mistakes like that, as people
tend to use the existing drivers as examples for newer ones.
compile-tested only.
Patches 1 to 7 fix some issues that already exists at the current
PM runtime code;
patches 8 to 20 fix some usage_count problems that still exists
at the media subsystem;
patches 21 to 78 repaces pm_runtime_get_sync() by
pm_runtime_resume_and_get();
Patch 79 (and a hunk on patch 78) documents the two exceptions
where pm_runtime_get_sync() will still be used for now.
---
v4:
- Added a couple of additional fixes at existing PM runtime code;
- Some patches are now more conservative in order to avoid causing
regressions.
v3:
- fix a compilation error;
v2:
- addressed pointed issues and fixed a few other PM issues.
Mauro Carvalho Chehab (79):
media: venus: fix PM runtime logic at venus_sys_error_handler()
media: s6p_cec: decrement usage count if disabled
media: i2c: ccs-core: return the right error code at suspend
media: i2c: ov7740: don't resume at remove time
media: i2c: video-i2c: don't resume at remove time
media: i2c: imx334: fix the pm runtime get logic
media: exynos-gsc: don't resume at remove time
media: atmel: properly get pm_runtime
media: marvel-ccic: fix some issues when getting pm_runtime
media: mdk-mdp: fix pm_runtime_get_sync() usage count
media: rcar_fdp1: fix pm_runtime_get_sync() usage count
media: renesas-ceu: Properly check for PM errors
media: s5p: fix pm_runtime_get_sync() usage count
media: am437x: fix pm_runtime_get_sync() usage count
media: sh_vou: fix pm_runtime_get_sync() usage count
media: mtk-vcodec: fix pm_runtime_get_sync() usage count
media: s5p-jpeg: fix pm_runtime_get_sync() usage count
media: sti/delta: fix pm_runtime_get_sync() usage count
media: sunxi: fix pm_runtime_get_sync() usage count
staging: media: rkvdec: fix pm_runtime_get_sync() usage count
staging: media: atomisp: use pm_runtime_resume_and_get()
staging: media: imx7-mipi-csis: use pm_runtime_resume_and_get()
staging: media: ipu3: use pm_runtime_resume_and_get()
staging: media: cedrus_video: use pm_runtime_resume_and_get()
staging: media: tegra-vde: use pm_runtime_resume_and_get()
staging: media: tegra-video: use pm_runtime_resume_and_get()
media: i2c: ak7375: use pm_runtime_resume_and_get()
media: i2c: ccs-core: use pm_runtime_resume_and_get()
media: i2c: dw9714: use pm_runtime_resume_and_get()
media: i2c: dw9768: use pm_runtime_resume_and_get()
media: i2c: dw9807-vcm: use pm_runtime_resume_and_get()
media: i2c: hi556: use pm_runtime_resume_and_get()
media: i2c: imx214: use pm_runtime_resume_and_get()
media: i2c: imx219: use pm_runtime_resume_and_get()
media: i2c: imx258: use pm_runtime_resume_and_get()
media: i2c: imx274: use pm_runtime_resume_and_get()
media: i2c: imx290: use pm_runtime_resume_and_get()
media: i2c: imx319: use pm_runtime_resume_and_get()
media: i2c: imx355: use pm_runtime_resume_and_get()
media: i2c: mt9m001: use pm_runtime_resume_and_get()
media: i2c: ov02a10: use pm_runtime_resume_and_get()
media: i2c: ov13858: use pm_runtime_resume_and_get()
media: i2c: ov2659: use pm_runtime_resume_and_get()
media: i2c: ov2685: use pm_runtime_resume_and_get()
media: i2c: ov2740: use pm_runtime_resume_and_get()
media: i2c: ov5647: use pm_runtime_resume_and_get()
media: i2c: ov5648: use pm_runtime_resume_and_get()
media: i2c: ov5670: use pm_runtime_resume_and_get()
media: i2c: ov5675: use pm_runtime_resume_and_get()
media: i2c: ov5695: use pm_runtime_resume_and_get()
media: i2c: ov7740: use pm_runtime_resume_and_get()
media: i2c: ov8856: use pm_runtime_resume_and_get()
media: i2c: ov8865: use pm_runtime_resume_and_get()
media: i2c: ov9734: use pm_runtime_resume_and_get()
media: i2c: tvp5150: use pm_runtime_resume_and_get()
media: i2c: video-i2c: use pm_runtime_resume_and_get()
media: rockchip/rga: use pm_runtime_resume_and_get()
media: sti/hva: use pm_runtime_resume_and_get()
media: sti/bdisp: use pm_runtime_resume_and_get()
media: ipu3: use pm_runtime_resume_and_get()
media: coda: use pm_runtime_resume_and_get()
media: exynos4-is: use pm_runtime_resume_and_get()
media: exynos-gsc: use pm_runtime_resume_and_get()
media: mtk-jpeg: use pm_runtime_resume_and_get()
media: camss: use pm_runtime_resume_and_get()
media: venus: use pm_runtime_resume_and_get()
media: venus: vdec: use pm_runtime_resume_and_get()
media: venus: venc: use pm_runtime_resume_and_get()
media: rcar-fcp: use pm_runtime_resume_and_get()
media: rkisp1: use pm_runtime_resume_and_get()
media: s3c-camif: use pm_runtime_resume_and_get()
media: s5p-mfc: use pm_runtime_resume_and_get()
media: stm32: use pm_runtime_resume_and_get()
media: sunxi: use pm_runtime_resume_and_get()
media: ti-vpe: use pm_runtime_resume_and_get()
media: vsp1: use pm_runtime_resume_and_get()
media: rcar-vin: use pm_runtime_resume_and_get()
media: hantro: use pm_runtime_resume_and_get()
media: hantro: do a PM resume earlier
drivers/media/cec/platform/s5p/s5p_cec.c | 7 +++--
drivers/media/i2c/ak7375.c | 10 +------
drivers/media/i2c/ccs/ccs-core.c | 18 +++++-------
drivers/media/i2c/dw9714.c | 10 +------
drivers/media/i2c/dw9768.c | 10 +------
drivers/media/i2c/dw9807-vcm.c | 10 +------
drivers/media/i2c/hi556.c | 3 +-
drivers/media/i2c/imx214.c | 6 ++--
drivers/media/i2c/imx219.c | 6 ++--
drivers/media/i2c/imx258.c | 6 ++--
drivers/media/i2c/imx274.c | 3 +-
drivers/media/i2c/imx290.c | 6 ++--
drivers/media/i2c/imx319.c | 6 ++--
drivers/media/i2c/imx334.c | 7 +++--
drivers/media/i2c/imx355.c | 6 ++--
drivers/media/i2c/mt9m001.c | 9 ++++--
drivers/media/i2c/ov02a10.c | 6 ++--
drivers/media/i2c/ov13858.c | 6 ++--
drivers/media/i2c/ov2659.c | 6 ++--
drivers/media/i2c/ov2685.c | 7 ++---
drivers/media/i2c/ov2740.c | 6 ++--
drivers/media/i2c/ov5647.c | 9 +++---
drivers/media/i2c/ov5648.c | 6 ++--
drivers/media/i2c/ov5670.c | 6 ++--
drivers/media/i2c/ov5675.c | 3 +-
drivers/media/i2c/ov5695.c | 6 ++--
drivers/media/i2c/ov7740.c | 8 ++---
drivers/media/i2c/ov8856.c | 3 +-
drivers/media/i2c/ov8865.c | 6 ++--
drivers/media/i2c/ov9734.c | 3 +-
drivers/media/i2c/tvp5150.c | 16 ++--------
drivers/media/i2c/video-i2c.c | 14 +++------
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 3 +-
drivers/media/platform/am437x/am437x-vpfe.c | 22 ++++++++++----
drivers/media/platform/atmel/atmel-isc-base.c | 27 ++++++++++++-----
drivers/media/platform/atmel/atmel-isi.c | 19 +++++++++---
drivers/media/platform/coda/coda-common.c | 7 +++--
drivers/media/platform/exynos-gsc/gsc-core.c | 11 +++----
drivers/media/platform/exynos-gsc/gsc-m2m.c | 2 +-
.../media/platform/exynos4-is/fimc-capture.c | 6 ++--
drivers/media/platform/exynos4-is/fimc-is.c | 4 +--
.../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 | 8 ++---
.../media/platform/marvell-ccic/mcam-core.c | 9 ++++--
.../media/platform/mtk-jpeg/mtk_jpeg_core.c | 4 +--
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 6 ++--
.../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 4 +--
.../media/platform/qcom/camss/camss-csid.c | 6 ++--
.../media/platform/qcom/camss/camss-csiphy.c | 6 ++--
.../media/platform/qcom/camss/camss-ispif.c | 6 ++--
drivers/media/platform/qcom/camss/camss-vfe.c | 5 ++--
drivers/media/platform/qcom/venus/core.c | 28 +++++++++++-------
.../media/platform/qcom/venus/pm_helpers.c | 10 +++----
drivers/media/platform/qcom/venus/vdec.c | 4 +--
drivers/media/platform/qcom/venus/venc.c | 5 ++--
drivers/media/platform/rcar-fcp.c | 6 ++--
drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++--
drivers/media/platform/rcar-vin/rcar-dma.c | 6 ++--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++--
drivers/media/platform/rcar_fdp1.c | 12 ++++++--
drivers/media/platform/renesas-ceu.c | 4 +--
drivers/media/platform/rockchip/rga/rga-buf.c | 3 +-
drivers/media/platform/rockchip/rga/rga.c | 4 ++-
.../platform/rockchip/rkisp1/rkisp1-capture.c | 3 +-
.../media/platform/s3c-camif/camif-capture.c | 2 +-
drivers/media/platform/s3c-camif/camif-core.c | 5 ++--
drivers/media/platform/s5p-jpeg/jpeg-core.c | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c | 6 ++--
drivers/media/platform/sh_vou.c | 6 +++-
drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 7 +++--
drivers/media/platform/sti/delta/delta-v4l2.c | 4 +--
drivers/media/platform/sti/hva/hva-hw.c | 17 ++++++-----
drivers/media/platform/stm32/stm32-dcmi.c | 5 ++--
.../platform/sunxi/sun4i-csi/sun4i_v4l2.c | 6 ++--
.../sunxi/sun8i-rotate/sun8i_rotate.c | 2 +-
drivers/media/platform/ti-vpe/cal-video.c | 4 ++-
drivers/media/platform/ti-vpe/cal.c | 8 +++--
drivers/media/platform/ti-vpe/vpe.c | 4 +--
drivers/media/platform/vsp1/vsp1_drv.c | 6 ++--
.../staging/media/atomisp/pci/atomisp_fops.c | 6 ++--
drivers/staging/media/hantro/hantro_drv.c | 29 ++++++++++++-------
drivers/staging/media/imx/imx7-mipi-csis.c | 7 ++---
drivers/staging/media/ipu3/ipu3.c | 3 +-
drivers/staging/media/rkvdec/rkvdec.c | 2 +-
.../staging/media/sunxi/cedrus/cedrus_video.c | 6 ++--
drivers/staging/media/tegra-vde/vde.c | 19 ++++++++++--
drivers/staging/media/tegra-video/csi.c | 3 +-
drivers/staging/media/tegra-video/vi.c | 3 +-
92 files changed, 335 insertions(+), 347 deletions(-)
--
2.30.2
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/i2c/ov8865.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 9ecf180635ee..3bf6ee4898a9 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2497,11 +2497,9 @@ static int ov8865_s_stream(struct v4l2_subdev *subdev, int enable)
int ret;
if (enable) {
- ret = pm_runtime_get_sync(sensor->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(sensor->dev);
+ ret = pm_runtime_resume_and_get(sensor->dev);
+ if (ret < 0)
return ret;
- }
}
mutex_lock(&sensor->mutex);
--
2.30.2
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.
Reviewed-by: Sylwester Nawrocki <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/exynos-gsc/gsc-m2m.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 27a3c92c73bc..09551e96ac15 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -58,7 +58,7 @@ static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
struct gsc_ctx *ctx = q->drv_priv;
int ret;
- ret = pm_runtime_get_sync(&ctx->gsc_dev->pdev->dev);
+ ret = pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
return ret > 0 ? 0 : ret;
}
--
2.30.2
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.
While here, as noted by Phillip, the labels at the coda_open()
function are currently named after what operation failed,
instead of what they do in response. So, change the name of
the error label that it is called when clk_enable fails,
in order to be consistent.
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/coda/coda-common.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index bd666c858fa1..2017de85713e 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2660,7 +2660,7 @@ static int coda_open(struct file *file)
ctx->use_vdoa = false;
/* Power up and upload firmware if necessary */
- ret = pm_runtime_get_sync(dev->dev);
+ ret = pm_runtime_resume_and_get(dev->dev);
if (ret < 0) {
v4l2_err(&dev->v4l2_dev, "failed to power up: %d\n", ret);
goto err_pm_get;
@@ -2668,7 +2668,7 @@ static int coda_open(struct file *file)
ret = clk_prepare_enable(dev->clk_per);
if (ret)
- goto err_pm_get;
+ goto err_clk_enable;
ret = clk_prepare_enable(dev->clk_ahb);
if (ret)
@@ -2707,8 +2707,9 @@ static int coda_open(struct file *file)
clk_disable_unprepare(dev->clk_ahb);
err_clk_ahb:
clk_disable_unprepare(dev->clk_per);
+err_clk_enable:
+ pm_runtime_put_sync(dev->dev);
err_pm_get:
- pm_runtime_put_sync(dev->dev);
v4l2_fh_del(&ctx->fh);
v4l2_fh_exit(&ctx->fh);
err_coda_name_init:
--
2.30.2
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.
Acked-by: Jacopo Mondi <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/i2c/ov5647.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 1cefa15729ce..38faa74755e3 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -882,20 +882,20 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
}
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
if (ret < 0)
goto error_unlock;
ret = ov5647_stream_on(sd);
if (ret < 0) {
dev_err(&client->dev, "stream start failed: %d\n", ret);
- goto error_unlock;
+ goto error_pm;
}
} else {
ret = ov5647_stream_off(sd);
if (ret < 0) {
dev_err(&client->dev, "stream stop failed: %d\n", ret);
- goto error_unlock;
+ goto error_pm;
}
pm_runtime_put(&client->dev);
}
@@ -905,8 +905,9 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
return 0;
+error_pm:
+ pm_runtime_put(&client->dev);
error_unlock:
- pm_runtime_put(&client->dev);
mutex_unlock(&sensor->lock);
return ret;
--
2.30.2
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/i2c/ov5670.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index dee7df8dd100..182f271f118f 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2347,11 +2347,9 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
goto unlock_and_return;
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto unlock_and_return;
- }
ret = ov5670_start_streaming(ov5670);
if (ret)
--
2.30.2
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/qcom/venus/venc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4a7291f934b6..8dd49d4f124c 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1205,9 +1205,9 @@ static int venc_open(struct file *file)
venus_helper_init_instance(inst);
- ret = pm_runtime_get_sync(core->dev_enc);
+ ret = pm_runtime_resume_and_get(core->dev_enc);
if (ret < 0)
- goto err_put_sync;
+ goto err_free;
ret = venc_ctrl_init(inst);
if (ret)
@@ -1252,6 +1252,7 @@ static int venc_open(struct file *file)
venc_ctrl_deinit(inst);
err_put_sync:
pm_runtime_put_sync(core->dev_enc);
+err_free:
kfree(inst);
return ret;
}
--
2.30.2
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/i2c/imx319.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 38540323a156..4e0a8c9d271f 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2141,11 +2141,9 @@ static int imx319_set_stream(struct v4l2_subdev *sd, int enable)
}
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto err_unlock;
- }
/*
* Apply default & customized values
--
2.30.2
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/i2c/hi556.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index 6f05c1138e3b..627ccfa34835 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -813,9 +813,8 @@ static int hi556_set_stream(struct v4l2_subdev *sd, int enable)
mutex_lock(&hi556->mutex);
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
mutex_unlock(&hi556->mutex);
return ret;
}
--
2.30.2
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/i2c/tvp5150.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index e26e3f544054..374a9da75e4d 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1448,11 +1448,9 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
TVP5150_MISC_CTL_CLOCK_OE;
if (enable) {
- ret = pm_runtime_get_sync(sd->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(sd->dev);
+ ret = pm_runtime_resume_and_get(sd->dev);
+ if (ret < 0)
return ret;
- }
tvp5150_enable(sd);
@@ -1675,15 +1673,7 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
static int tvp5150_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
{
- int ret;
-
- ret = pm_runtime_get_sync(sd->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(sd->dev);
- return ret;
- }
-
- return 0;
+ return pm_runtime_resume_and_get(sd->dev);
}
static int tvp5150_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
--
2.30.2
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/stm32/stm32-dcmi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index bbcc2254fa2e..5f4e1db8cfcd 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -723,11 +723,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
u32 val = 0;
int ret;
- ret = pm_runtime_get_sync(dcmi->dev);
+ ret = pm_runtime_resume_and_get(dcmi->dev);
if (ret < 0) {
dev_err(dcmi->dev, "%s: Failed to start streaming, cannot get sync (%d)\n",
__func__, ret);
- goto err_pm_put;
+ goto err_unlock;
}
ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
@@ -848,6 +848,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
err_pm_put:
pm_runtime_put(dcmi->dev);
+err_unlock:
spin_lock_irq(&dcmi->irqlock);
/*
* Return all buffers to vb2 in QUEUED state.
--
2.30.2
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/i2c/ov5675.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index dea32859459a..e7e297a23960 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -863,9 +863,8 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
mutex_lock(&ov5675->mutex);
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
mutex_unlock(&ov5675->mutex);
return ret;
}
--
2.30.2
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.
While the hva driver does it right, there are lots of errors
on other drivers due to its misusage. So, let's change
this driver to also use pm_runtime_resume_and_get(), as we're
doing similar changes all over the media subsystem.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/sti/hva/hva-hw.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/sti/hva/hva-hw.c b/drivers/media/platform/sti/hva/hva-hw.c
index f59811e27f51..77b8bfa5e0c5 100644
--- a/drivers/media/platform/sti/hva/hva-hw.c
+++ b/drivers/media/platform/sti/hva/hva-hw.c
@@ -270,9 +270,8 @@ static unsigned long int hva_hw_get_ip_version(struct hva_dev *hva)
struct device *dev = hva_to_dev(hva);
unsigned long int version;
- if (pm_runtime_get_sync(dev) < 0) {
+ if (pm_runtime_resume_and_get(dev) < 0) {
dev_err(dev, "%s failed to get pm_runtime\n", HVA_PREFIX);
- pm_runtime_put_noidle(dev);
mutex_unlock(&hva->protect_mutex);
return -EFAULT;
}
@@ -386,10 +385,10 @@ int hva_hw_probe(struct platform_device *pdev, struct hva_dev *hva)
pm_runtime_set_suspended(dev);
pm_runtime_enable(dev);
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0) {
dev_err(dev, "%s failed to set PM\n", HVA_PREFIX);
- goto err_pm;
+ goto err_clk;
}
/* check IP hardware version */
@@ -462,6 +461,7 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
u8 client_id = ctx->id;
int ret;
u32 reg = 0;
+ bool got_pm = false;
mutex_lock(&hva->protect_mutex);
@@ -469,12 +469,13 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
enable_irq(hva->irq_its);
enable_irq(hva->irq_err);
- if (pm_runtime_get_sync(dev) < 0) {
+ if (pm_runtime_resume_and_get(dev) < 0) {
dev_err(dev, "%s failed to get pm_runtime\n", ctx->name);
ctx->sys_errors++;
ret = -EFAULT;
goto out;
}
+ got_pm = true;
reg = readl_relaxed(hva->regs + HVA_HIF_REG_CLK_GATING);
switch (cmd) {
@@ -537,7 +538,8 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
dev_dbg(dev, "%s unknown command 0x%x\n", ctx->name, cmd);
}
- pm_runtime_put_autosuspend(dev);
+ if (got_pm)
+ pm_runtime_put_autosuspend(dev);
mutex_unlock(&hva->protect_mutex);
return ret;
@@ -553,9 +555,8 @@ void hva_hw_dump_regs(struct hva_dev *hva, struct seq_file *s)
mutex_lock(&hva->protect_mutex);
- if (pm_runtime_get_sync(dev) < 0) {
+ if (pm_runtime_resume_and_get(dev) < 0) {
seq_puts(s, "Cannot wake up IP\n");
- pm_runtime_put_noidle(dev);
mutex_unlock(&hva->protect_mutex);
return;
}
--
2.30.2
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/staging/media/tegra-vde/vde.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index 28845b5bafaf..1cdacb3f781c 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,11 +1071,17 @@ 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);
+ if (pm_runtime_resume_and_get(dev) < 0)
+ goto err_pm_runtime;
+
pm_runtime_put(dev);
return 0;
+err_pm_runtime:
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_runtime_disable(dev);
+
err_deinit_iommu:
tegra_vde_iommu_deinit(vde);
@@ -1089,7 +1097,12 @@ static int tegra_vde_remove(struct platform_device *pdev)
struct tegra_vde *vde = platform_get_drvdata(pdev);
struct device *dev = &pdev->dev;
+ /*
+ * As it increments RPM usage_count even on errors, we don't need to
+ * check the returned code here.
+ */
pm_runtime_get_sync(dev);
+
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
--
2.30.2
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/i2c/ov5695.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
index 09bee57a241d..469d941813c6 100644
--- a/drivers/media/i2c/ov5695.c
+++ b/drivers/media/i2c/ov5695.c
@@ -946,11 +946,9 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
goto unlock_and_return;
if (on) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto unlock_and_return;
- }
ret = __ov5695_start_stream(ov5695);
if (ret) {
--
2.30.2
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/i2c/ov2659.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index 42f64175a6df..a3c8eae68486 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1186,11 +1186,9 @@ static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
goto unlock;
}
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto unlock;
- }
ret = ov2659_init(sd, 0);
if (!ret)
--
2.30.2
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/rockchip/rkisp1/rkisp1-capture.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 5f6c9d1623e4..3730376897d9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -1003,9 +1003,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
if (ret)
goto err_pipeline_stop;
- ret = pm_runtime_get_sync(cap->rkisp1->dev);
+ ret = pm_runtime_resume_and_get(cap->rkisp1->dev);
if (ret < 0) {
- pm_runtime_put_noidle(cap->rkisp1->dev);
dev_err(cap->rkisp1->dev, "power up failed %d\n", ret);
goto err_destroy_dummy;
}
--
2.30.2
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/i2c/imx274.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index cdccaab3043a..ee2127436f0b 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1441,9 +1441,8 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
mutex_lock(&imx274->lock);
if (on) {
- ret = pm_runtime_get_sync(&imx274->client->dev);
+ ret = pm_runtime_resume_and_get(&imx274->client->dev);
if (ret < 0) {
- pm_runtime_put_noidle(&imx274->client->dev);
mutex_unlock(&imx274->lock);
return ret;
}
--
2.30.2
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/i2c/imx219.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 1054ffedaefd..74a0bf9b088b 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1035,11 +1035,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
const struct imx219_reg_list *reg_list;
int ret;
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
return ret;
- }
/* Apply default values of current mode */
reg_list = &imx219->mode->reg_list;
--
2.30.2
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/i2c/imx355.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index ccedcd4c520a..93f13a04439a 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -1442,11 +1442,9 @@ static int imx355_set_stream(struct v4l2_subdev *sd, int enable)
}
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto err_unlock;
- }
/*
* Apply default & customized values
--
2.30.2
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.
Reviewed-by: Sylwester Nawrocki <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/s3c-camif/camif-capture.c | 2 +-
drivers/media/platform/s3c-camif/camif-core.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 9ca49af29542..62241ec3b978 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -547,7 +547,7 @@ static int s3c_camif_open(struct file *file)
if (ret < 0)
goto unlock;
- ret = pm_runtime_get_sync(camif->dev);
+ ret = pm_runtime_resume_and_get(camif->dev);
if (ret < 0)
goto err_pm;
diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
index 4c3c00d59c92..e1d51fd3e700 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -460,9 +460,9 @@ static int s3c_camif_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
- goto err_pm;
+ goto err_disable;
ret = camif_media_dev_init(camif);
if (ret < 0)
@@ -502,6 +502,7 @@ static int s3c_camif_probe(struct platform_device *pdev)
camif_unregister_media_entities(camif);
err_pm:
pm_runtime_put(dev);
+err_disable:
pm_runtime_disable(dev);
camif_clk_put(camif);
err_clk:
--
2.30.2
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/i2c/ov7740.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
index ed6904b2e8f5..74219f67f245 100644
--- a/drivers/media/i2c/ov7740.c
+++ b/drivers/media/i2c/ov7740.c
@@ -624,11 +624,9 @@ static int ov7740_set_stream(struct v4l2_subdev *sd, int enable)
}
if (enable) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&client->dev);
+ ret = pm_runtime_resume_and_get(&client->dev);
+ if (ret < 0)
goto err_unlock;
- }
ret = ov7740_start_streaming(ov7740);
if (ret)
--
2.30.2
Calling pm_runtime_get_sync() at driver's removal time is
not right, as this will resume PM runtime. This is clearly
not what it is desired there, since it calls
pm_runtime_set_suspended() afterwards.
So, just remove pm runtime get/put logic from the device
removal logic.
Fixes: 69d2a734c5dc ("media: video-i2c: support runtime PM")
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/i2c/video-i2c.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 0465832a4090..c9a774f4c8d2 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -893,10 +893,8 @@ static int video_i2c_remove(struct i2c_client *client)
{
struct video_i2c_data *data = i2c_get_clientdata(client);
- pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
- pm_runtime_put_noidle(&client->dev);
if (data->chip->set_power)
data->chip->set_power(data, false);
--
2.30.2
The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter and avoid memory
leaks.
Reviewed-by: Sylwester Nawrocki <[email protected]>
Acked-by: Andrzej Pietrasiewicz <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 026111505f5a..c4f19418a460 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2568,7 +2568,7 @@ static int s5p_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
int ret;
- ret = pm_runtime_get_sync(ctx->jpeg->dev);
+ ret = pm_runtime_resume_and_get(ctx->jpeg->dev);
return ret > 0 ? 0 : ret;
}
--
2.30.2
The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter and avoid memory
leaks.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
index ddee7046ce42..fe096fe61c9d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -92,9 +92,9 @@ void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
{
int ret;
- ret = pm_runtime_get_sync(pm->dev);
+ ret = pm_runtime_resume_and_get(pm->dev);
if (ret)
- mtk_v4l2_err("pm_runtime_get_sync fail %d", ret);
+ mtk_v4l2_err("pm_runtime_resume_and_get fail %d", ret);
}
void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
--
2.30.2
The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter and avoid memory
leaks.
Reviewed-by: Ezequiel Garcia <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/staging/media/rkvdec/rkvdec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index d821661d30f3..8c17615f3a7a 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -658,7 +658,7 @@ static void rkvdec_device_run(void *priv)
if (WARN_ON(!desc))
return;
- ret = pm_runtime_get_sync(rkvdec->dev);
+ ret = pm_runtime_resume_and_get(rkvdec->dev);
if (ret < 0) {
rkvdec_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
return;
--
2.30.2
On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> During the review of the patches from unm.edu, one of the patterns
> I noticed is the amount of patches trying to fix pm_runtime_get_sync()
> calls.
>
> After analyzing the feedback from version 1 of this series, I noticed
> a few other weird behaviors at the PM runtime resume code. So, this
> series start addressing some bugs and issues at the current code.
> Then, it gets rid of pm_runtime_get_sync() at the media subsystem
> (with 2 exceptions).
>
> It should be noticed that
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> added a new method to does a pm_runtime get, which increments
> the usage count only on success.
>
> The rationale of getting rid of pm_runtime_get_sync() is:
>
> 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 ;-)
This is perfectly alright. Take a look at ov7740_remove() for example:
pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
ov7740_set_power(ov7740, 0);
There's an explicit power-off after balancing the PM count and this will
work regardless of the power state when entering this function.
So this has nothing to do with pm_runtime_get_sync() per se.
> 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;
Sure, but pm_runtime_get_async() also works this way. You just won't be
notified if the async resume fails.
> 3. The name of the new variant is a lot clearer:
> pm_runtime_resume_and_get()
> As its same clearly says that this is a PM runtime resume function,
> that also increments the usage counter on success;
It also introduced an inconsistency in the API and does not pair as well
with the pm_runtime_put variants.
> 4. 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;
It's not known to be safe; there are ways to get also this interface
wrong as for example this series has shown.
> 5. 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.
Cargo-cult programming always runs that risk.
> This series replace places pm_runtime_get_sync(), by calling
> pm_runtime_resume_and_get() instead.
>
> This should help to avoid future mistakes like that, as people
> tend to use the existing drivers as examples for newer ones.
The only valid point about and use for pm_runtime_resume_and_get() is to
avoid leaking a PM usage count reference in the unlikely case that
resume fails (something which hardly any driver implements recovery
from anyway).
It's a convenience wrapper that saves you from writing one extra line in
some cases (depending on how you implement runtime-pm support) and not a
silver bullet against bugs.
> compile-tested only.
>
> Patches 1 to 7 fix some issues that already exists at the current
> PM runtime code;
>
> patches 8 to 20 fix some usage_count problems that still exists
> at the media subsystem;
>
> patches 21 to 78 repaces pm_runtime_get_sync() by
> pm_runtime_resume_and_get();
>
> Patch 79 (and a hunk on patch 78) documents the two exceptions
> where pm_runtime_get_sync() will still be used for now.
>
> ---
>
> v4:
> - Added a couple of additional fixes at existing PM runtime code;
> - Some patches are now more conservative in order to avoid causing
> regressions.
> v3:
> - fix a compilation error;
> v2:
> - addressed pointed issues and fixed a few other PM issues.
This really doesn't say much more than "changed stuff" so kinda hard to
track if review feedback has been taken into account for example.
Johan
Em Wed, 28 Apr 2021 17:50:08 +0200
Johan Hovold <[email protected]> escreveu:
> On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> > 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 ;-)
>
> This is perfectly alright. Take a look at ov7740_remove() for example:
>
> pm_runtime_get_sync(&client->dev);
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
> pm_runtime_put_noidle(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> There's an explicit power-off after balancing the PM count and this will
> work regardless of the power state when entering this function.
Ok, but this should equally work:
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
ov7740_set_power(ov7740, 0);
as there's no additional cleanup made on this particular driver
between pm_runtime_get_sync() and pm_runtime_put_noidle().
> So this has nothing to do with pm_runtime_get_sync() per se.
Yes, but some patches on this series are cleaning up the driver release
logic.
>
> > 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;
>
> Sure, but pm_runtime_get_async() also works this way. You just won't be
> notified if the async resume fails.
Granted, it makes sense along the pm_runtime kAPI.
It is inconsistent with the behavior of kobject_get*() and other
*_get*() methods that are based or inspired on it, as, on those, the
operations are atomic: either everything succeeds and it doesn't return
an error, or the usage counter is not incremented and the object
state doesn't change after the call.
> > 3. The name of the new variant is a lot clearer:
> > pm_runtime_resume_and_get()
> > As its same clearly says that this is a PM runtime resume function,
> > that also increments the usage counter on success;
>
> It also introduced an inconsistency in the API and does not pair as well
> with the pm_runtime_put variants.
Agreed. A name that would be more consistent with PM runtime would
probably be:
pm_runtime_resume_if_get()
as there are already:
pm_runtime_get_if_in_use()
pm_runtime_get_if_active()
But any such discussions are out of the scope of this patchset ;-)
>
> > 4. 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;
>
> It's not known to be safe; there are ways to get also this interface
> wrong as for example this series has shown.
Very true. Yet, it is a lot simpler to use functions that won't change
the state of the objects when returning an error, as this is by far
the most common pattern within the Kernel.
Human brains are trained to identify certain patterns. When there's
something using a similar pattern, but with a different behavior,
our brains are more subject to fail identifying problems.
> > 5. 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.
>
> Cargo-cult programming always runs that risk.
True.
> > This series replace places pm_runtime_get_sync(), by calling
> > pm_runtime_resume_and_get() instead.
> >
> > This should help to avoid future mistakes like that, as people
> > tend to use the existing drivers as examples for newer ones.
>
> The only valid point about and use for pm_runtime_resume_and_get() is to
> avoid leaking a PM usage count reference in the unlikely case that
> resume fails (something which hardly any driver implements recovery
> from anyway).
>
> It's a convenience wrapper that saves you from writing one extra line in
> some cases (depending on how you implement runtime-pm support) and not a
> silver bullet against bugs.
>
> > compile-tested only.
> >
> > Patches 1 to 7 fix some issues that already exists at the current
> > PM runtime code;
> >
> > patches 8 to 20 fix some usage_count problems that still exists
> > at the media subsystem;
> >
> > patches 21 to 78 repaces pm_runtime_get_sync() by
> > pm_runtime_resume_and_get();
> >
> > Patch 79 (and a hunk on patch 78) documents the two exceptions
> > where pm_runtime_get_sync() will still be used for now.
> >
> > ---
> >
> > v4:
> > - Added a couple of additional fixes at existing PM runtime code;
> > - Some patches are now more conservative in order to avoid causing
> > regressions.
> > v3:
> > - fix a compilation error;
> > v2:
> > - addressed pointed issues and fixed a few other PM issues.
>
> This really doesn't say much more than "changed stuff" so kinda hard to
> track if review feedback has been taken into account for example.
I addressed all review feedback I got (as far as I'm aware), and added
all received reviewed-by/acked-by.
Yeah, I could have written a more comprehensive changes description
there.
Thanks,
Mauro
29.04.2021 13:18, Mauro Carvalho Chehab пишет:
>> This is perfectly alright. Take a look at ov7740_remove() for example:
>>
>> pm_runtime_get_sync(&client->dev);
>> pm_runtime_disable(&client->dev);
>> pm_runtime_set_suspended(&client->dev);
>> pm_runtime_put_noidle(&client->dev);
>>
>> ov7740_set_power(ov7740, 0);
>>
>> There's an explicit power-off after balancing the PM count and this will
>> work regardless of the power state when entering this function.
> Ok, but this should equally work:
>
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> as there's no additional cleanup made on this particular driver
> between pm_runtime_get_sync() and pm_runtime_put_noidle().
>
The pm_runtime_get_sync() turns hardware ON by invoking
ov7740_set_power(ov7740, 1), and thus, the ON->OFF is kept balanced in
both RPM-available and RPM-unavailable cases. The RPM state of device
should be reset after driver removal.
It doesn't look like any additional cleanups are needed by that ov7740
driver. The driver removal is opposite to the probe, hence it should be
correct as-is.
28.04.2021 17:51, Mauro Carvalho Chehab пишет:
> @@ -1069,11 +1071,17 @@ 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);
> + if (pm_runtime_resume_and_get(dev) < 0)
> + goto err_pm_runtime;
> +
> pm_runtime_put(dev);
>
> return 0;
>
> +err_pm_runtime:
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> +
> err_deinit_iommu:
> tegra_vde_iommu_deinit(vde);
This is incorrect error unwinding, the miscdev isn't unregistered.
On Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 17:50:08 +0200
> Johan Hovold <[email protected]> escreveu:
>
> > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
>
> > > 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 ;-)
> >
> > This is perfectly alright. Take a look at ov7740_remove() for example:
> >
> > pm_runtime_get_sync(&client->dev);
> > pm_runtime_disable(&client->dev);
> > pm_runtime_set_suspended(&client->dev);
> > pm_runtime_put_noidle(&client->dev);
> >
> > ov7740_set_power(ov7740, 0);
> >
> > There's an explicit power-off after balancing the PM count and this will
> > work regardless of the power state when entering this function.
>
> Ok, but this should equally work:
>
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> as there's no additional cleanup made on this particular driver
> between pm_runtime_get_sync() and pm_runtime_put_noidle().
No, that would break the driver as I pointed out to you yesterday:
https://lore.kernel.org/r/[email protected]
If the device is already suspended when remove is called then you'll
end up with an unbalanced call to ov7740_set_power() that will try to
disable an already disabled clock.
> > So this has nothing to do with pm_runtime_get_sync() per se.
>
> Yes, but some patches on this series are cleaning up the driver release
> logic.
You mentioned this example as an argument against using
pm_runtime_get_sync(), which I don't think makes sense.
> > > 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;
> >
> > Sure, but pm_runtime_get_async() also works this way. You just won't be
> > notified if the async resume fails.
>
> Granted, it makes sense along the pm_runtime kAPI.
>
> It is inconsistent with the behavior of kobject_get*() and other
> *_get*() methods that are based or inspired on it, as, on those, the
> operations are atomic: either everything succeeds and it doesn't return
> an error, or the usage counter is not incremented and the object
> state doesn't change after the call.
Right, and I'm aware that some people have overlooked this. But its not
the end of the world since hardly any driver can handle resume failures
properly anyway.
This is mostly just an exercise to shut up static checkers.
> > > 3. The name of the new variant is a lot clearer:
> > > pm_runtime_resume_and_get()
> > > As its same clearly says that this is a PM runtime resume function,
> > > that also increments the usage counter on success;
> >
> > It also introduced an inconsistency in the API and does not pair as well
> > with the pm_runtime_put variants.
>
> Agreed. A name that would be more consistent with PM runtime would
> probably be:
>
> pm_runtime_resume_if_get()
Naw, since the get part always succeeds.
It should start with pm_runtime_get, but pm_runtime_get_sync() is
unfortunately taken.
> as there are already:
>
> pm_runtime_get_if_in_use()
> pm_runtime_get_if_active()
>
> But any such discussions are out of the scope of this patchset ;-)
Right.
> > > 4. 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;
> >
> > It's not known to be safe; there are ways to get also this interface
> > wrong as for example this series has shown.
>
> Very true. Yet, it is a lot simpler to use functions that won't change
> the state of the objects when returning an error, as this is by far
> the most common pattern within the Kernel.
A resume failure does change the state (and needs to be recovered from),
but I get what you're saying.
> Human brains are trained to identify certain patterns. When there's
> something using a similar pattern, but with a different behavior,
> our brains are more subject to fail identifying problems.
Sure. But I'm not sure that having two interfaces with different
semantics to do the job is doing us any favours here. But again, that
discussion has already been had.
And I realise that this is partly also your motive here (even if the old
interface isn't going to go away).
> > > compile-tested only.
> > > Patches 1 to 7 fix some issues that already exists at the current
> > > PM runtime code;
> > >
> > > patches 8 to 20 fix some usage_count problems that still exists
> > > at the media subsystem;
> > >
> > > patches 21 to 78 repaces pm_runtime_get_sync() by
> > > pm_runtime_resume_and_get();
> > >
> > > Patch 79 (and a hunk on patch 78) documents the two exceptions
> > > where pm_runtime_get_sync() will still be used for now.
80 patches in one series (posted to lkml) is a bit excessive. Perhaps
you can break it up in a fixes part and one or more cleanups parts?
Johan
On Wed, 28 Apr 2021 16:51:37 +0200
Mauro Carvalho Chehab <[email protected]> wrote:
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter and avoid memory
> leaks.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Without a return value, this seems like it's not complete as driver
will carry on and eventually call mtk_vcode_dec_pw_off() which will assume
it needs to decrement the count.
> ---
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> index ddee7046ce42..fe096fe61c9d 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> @@ -92,9 +92,9 @@ void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> {
> int ret;
>
> - ret = pm_runtime_get_sync(pm->dev);
> + ret = pm_runtime_resume_and_get(pm->dev);
> if (ret)
> - mtk_v4l2_err("pm_runtime_get_sync fail %d", ret);
> + mtk_v4l2_err("pm_runtime_resume_and_get fail %d", ret);
> }
>
> void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
On Wed, 28 Apr 2021 16:51:46 +0200
Mauro Carvalho Chehab <[email protected]> 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]>
LGTM
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/staging/media/tegra-vde/vde.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
> index 28845b5bafaf..1cdacb3f781c 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,11 +1071,17 @@ 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);
> + if (pm_runtime_resume_and_get(dev) < 0)
> + goto err_pm_runtime;
> +
> pm_runtime_put(dev);
>
> return 0;
>
> +err_pm_runtime:
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> +
> err_deinit_iommu:
> tegra_vde_iommu_deinit(vde);
>
> @@ -1089,7 +1097,12 @@ static int tegra_vde_remove(struct platform_device *pdev)
> struct tegra_vde *vde = platform_get_drvdata(pdev);
> struct device *dev = &pdev->dev;
>
> + /*
> + * As it increments RPM usage_count even on errors, we don't need to
> + * check the returned code here.
> + */
> pm_runtime_get_sync(dev);
> +
> pm_runtime_dont_use_autosuspend(dev);
> pm_runtime_disable(dev);
>
On Fri, 30 Apr 2021 18:08:36 +0100
Jonathan Cameron <[email protected]> wrote:
> On Wed, 28 Apr 2021 16:51:46 +0200
> Mauro Carvalho Chehab <[email protected]> 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]>
> LGTM
>
> Reviewed-by: Jonathan Cameron <[email protected]>
drop that. I missed the misc unwind thing caught in the other review.
Too many patches without a break :(
>
> > ---
> > drivers/staging/media/tegra-vde/vde.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
> > index 28845b5bafaf..1cdacb3f781c 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,11 +1071,17 @@ 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);
> > + if (pm_runtime_resume_and_get(dev) < 0)
> > + goto err_pm_runtime;
> > +
> > pm_runtime_put(dev);
> >
> > return 0;
> >
> > +err_pm_runtime:
> > + pm_runtime_dont_use_autosuspend(dev);
> > + pm_runtime_disable(dev);
> > +
> > err_deinit_iommu:
> > tegra_vde_iommu_deinit(vde);
> >
> > @@ -1089,7 +1097,12 @@ static int tegra_vde_remove(struct platform_device *pdev)
> > struct tegra_vde *vde = platform_get_drvdata(pdev);
> > struct device *dev = &pdev->dev;
> >
> > + /*
> > + * As it increments RPM usage_count even on errors, we don't need to
> > + * check the returned code here.
> > + */
> > pm_runtime_get_sync(dev);
> > +
> > pm_runtime_dont_use_autosuspend(dev);
> > pm_runtime_disable(dev);
> >
>
On Wed, 28 Apr 2021 16:52:19 +0200
Mauro Carvalho Chehab <[email protected]> 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.
>
> While the hva driver does it right, there are lots of errors
> on other drivers due to its misusage. So, let's change
> this driver to also use pm_runtime_resume_and_get(), as we're
> doing similar changes all over the media subsystem.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Bit of a fiddly one, but looks right to me.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/media/platform/sti/hva/hva-hw.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/sti/hva/hva-hw.c b/drivers/media/platform/sti/hva/hva-hw.c
> index f59811e27f51..77b8bfa5e0c5 100644
> --- a/drivers/media/platform/sti/hva/hva-hw.c
> +++ b/drivers/media/platform/sti/hva/hva-hw.c
> @@ -270,9 +270,8 @@ static unsigned long int hva_hw_get_ip_version(struct hva_dev *hva)
> struct device *dev = hva_to_dev(hva);
> unsigned long int version;
>
> - if (pm_runtime_get_sync(dev) < 0) {
> + if (pm_runtime_resume_and_get(dev) < 0) {
> dev_err(dev, "%s failed to get pm_runtime\n", HVA_PREFIX);
> - pm_runtime_put_noidle(dev);
> mutex_unlock(&hva->protect_mutex);
> return -EFAULT;
> }
> @@ -386,10 +385,10 @@ int hva_hw_probe(struct platform_device *pdev, struct hva_dev *hva)
> pm_runtime_set_suspended(dev);
> pm_runtime_enable(dev);
>
> - ret = pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
> if (ret < 0) {
> dev_err(dev, "%s failed to set PM\n", HVA_PREFIX);
> - goto err_pm;
> + goto err_clk;
> }
>
> /* check IP hardware version */
> @@ -462,6 +461,7 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
> u8 client_id = ctx->id;
> int ret;
> u32 reg = 0;
> + bool got_pm = false;
>
> mutex_lock(&hva->protect_mutex);
>
> @@ -469,12 +469,13 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
> enable_irq(hva->irq_its);
> enable_irq(hva->irq_err);
>
> - if (pm_runtime_get_sync(dev) < 0) {
> + if (pm_runtime_resume_and_get(dev) < 0) {
> dev_err(dev, "%s failed to get pm_runtime\n", ctx->name);
> ctx->sys_errors++;
> ret = -EFAULT;
> goto out;
> }
> + got_pm = true;
>
> reg = readl_relaxed(hva->regs + HVA_HIF_REG_CLK_GATING);
> switch (cmd) {
> @@ -537,7 +538,8 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum hva_hw_cmd_type cmd,
> dev_dbg(dev, "%s unknown command 0x%x\n", ctx->name, cmd);
> }
>
> - pm_runtime_put_autosuspend(dev);
> + if (got_pm)
> + pm_runtime_put_autosuspend(dev);
> mutex_unlock(&hva->protect_mutex);
>
> return ret;
> @@ -553,9 +555,8 @@ void hva_hw_dump_regs(struct hva_dev *hva, struct seq_file *s)
>
> mutex_lock(&hva->protect_mutex);
>
> - if (pm_runtime_get_sync(dev) < 0) {
> + if (pm_runtime_resume_and_get(dev) < 0) {
> seq_puts(s, "Cannot wake up IP\n");
> - pm_runtime_put_noidle(dev);
> mutex_unlock(&hva->protect_mutex);
> return;
> }
On Wed, 28 Apr 2021 16:52:24 +0200
Mauro Carvalho Chehab <[email protected]> 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.
>
> Reviewed-by: Sylwester Nawrocki <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/platform/exynos-gsc/gsc-m2m.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 27a3c92c73bc..09551e96ac15 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -58,7 +58,7 @@ static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
> struct gsc_ctx *ctx = q->drv_priv;
> int ret;
>
> - ret = pm_runtime_get_sync(&ctx->gsc_dev->pdev->dev);
> + ret = pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
> return ret > 0 ? 0 : ret;
return pm_runtime_resume_and_get()
as
static inline int pm_runtime_resume_and_get(struct device *dev)
{
int ret;
ret = __pm_runtime_resume(dev, RPM_GET_PUT);
if (ret < 0) {
pm_runtime_put_noidle(dev);
return ret;
}
return 0;
}
Can't return >= 0
> }
>
On Wed, 28 Apr 2021 16:52:34 +0200
Mauro Carvalho Chehab <[email protected]> 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/stm32/stm32-dcmi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index bbcc2254fa2e..5f4e1db8cfcd 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -723,11 +723,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> u32 val = 0;
> int ret;
>
> - ret = pm_runtime_get_sync(dcmi->dev);
> + ret = pm_runtime_resume_and_get(dcmi->dev);
> if (ret < 0) {
> dev_err(dcmi->dev, "%s: Failed to start streaming, cannot get sync (%d)\n",
> __func__, ret);
> - goto err_pm_put;
> + goto err_unlock;
maybe err_unlocked; to indicate the lock isn't held. This briefly confused me.
> }
>
> ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
> @@ -848,6 +848,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>
> err_pm_put:
> pm_runtime_put(dcmi->dev);
> +err_unlock:
> spin_lock_irq(&dcmi->irqlock);
> /*
> * Return all buffers to vb2 in QUEUED state.
Hi Mauro,
Thank you for the patch.
On Wed, Apr 28, 2021 at 3:52 PM Mauro Carvalho Chehab
<[email protected]> 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/i2c/ov2659.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
Acked-by: Lad Prabhakar <[email protected]>
Cheers,
Prabhakar
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> index 42f64175a6df..a3c8eae68486 100644
> --- a/drivers/media/i2c/ov2659.c
> +++ b/drivers/media/i2c/ov2659.c
> @@ -1186,11 +1186,9 @@ static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
> goto unlock;
> }
>
> - ret = pm_runtime_get_sync(&client->dev);
> - if (ret < 0) {
> - pm_runtime_put_noidle(&client->dev);
> + ret = pm_runtime_resume_and_get(&client->dev);
> + if (ret < 0)
> goto unlock;
> - }
>
> ret = ov2659_init(sd, 0);
> if (!ret)
> --
> 2.30.2
>