[PATCH 00/78] media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()

[PATCH 00/78] media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()

[Mauro Carvalho Chehab]

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.

On contrary of the common sense that a foo_get() function will
only increment the usage on success,  pm_runtime_get_sync()
increments it unconditionally.

Due to that, there are bugs on lots of places, that ended being
gradually fixed, but, still there are a few places on media where
this is still broken.

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.

This series replace all places where the old  pm_runtime_get_sync()
is called, using  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.

Mauro Carvalho Chehab (78):
  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  usage count
  media: mdk-mdp: fix pm_runtime_get_sync() usage count
  media: renesas-ceu: fix pm_runtime_get_sync() usage count
  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: sti/hva: use pm_runtime_resume_and_get()
  staging: media: rkvdec: fix pm_runtime_get_sync() usage count
  staging: media: atomisp_fops: use pm_runtime_resume_and_get()
  staging: media: hantro_drv: 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: vde: use pm_runtime_resume_and_get()
  staging: media: csi: use pm_runtime_resume_and_get()
  staging: media: vi: use pm_runtime_resume_and_get()
  media: mtk-vcodec: fix pm_runtime_get_sync() usage count
  media: s5p-jpeg: fix pm_runtime_get_sync() usage count
  media: delta-v4l2: fix pm_runtime_get_sync() usage count
  media: sun8i_rotate: fix pm_runtime_get_sync() usage count
  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: imx334: 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: 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-csid: use pm_runtime_resume_and_get()
  media: camss-csiphy: use pm_runtime_resume_and_get()
  media: camss-ispif: use pm_runtime_resume_and_get()
  media: camss-vfe: use pm_runtime_resume_and_get()
  media: core: use pm_runtime_resume_and_get()
  media: pm_helpers: use pm_runtime_resume_and_get()
  media: vdec: use pm_runtime_resume_and_get()
  media: venc: use pm_runtime_resume_and_get()
  media: rcar-fcp: use pm_runtime_resume_and_get()
  media: rcar-vin: use pm_runtime_resume_and_get()
  media: rga-buf: use pm_runtime_resume_and_get()
  media: rkisp1-capture: 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: bdisp-v4l2: use pm_runtime_resume_and_get()
  media: stm32: use pm_runtime_resume_and_get()
  media: sun4i_v4l2: use pm_runtime_resume_and_get()
  media: ti-vpe: use pm_runtime_resume_and_get()
  media: vsp1: use pm_runtime_resume_and_get()

 drivers/media/cec/platform/s5p/s5p_cec.c      |  5 +++-
 drivers/media/i2c/ak7375.c                    | 10 +------
 drivers/media/i2c/ccs/ccs-core.c              | 11 ++++----
 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                    |  5 ++--
 drivers/media/i2c/imx355.c                    |  6 ++---
 drivers/media/i2c/mt9m001.c                   |  7 ++---
 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   | 10 ++++---
 drivers/media/platform/atmel/atmel-isc-base.c | 26 ++++++++++++++-----
 drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++++---
 drivers/media/platform/coda/coda-common.c     |  2 +-
 drivers/media/platform/exynos-gsc/gsc-core.c  |  3 +--
 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   |  3 ++-
 .../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 ++--
 .../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      | 19 +++++++-------
 .../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   |  2 +-
 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          |  5 +++-
 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  |  5 ++--
 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     |  7 +++--
 .../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     |  2 +-
 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         | 16 +++++++-----
 drivers/staging/media/tegra-video/csi.c       |  3 +--
 drivers/staging/media/tegra-video/vi.c        |  3 +--
 92 files changed, 270 insertions(+), 322 deletions(-)

-- 
2.30.2

[PATCH 07/78] media: s5p: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

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.

While here, check if the PM runtime was caught at
s5p_cec_adap_enable().

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/cec/platform/s5p/s5p_cec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/cec/platform/s5p/s5p_cec.c b/drivers/media/cec/platform/s5p/s5p_cec.c
index 2a3e7ffefe0a..2250c1cbc64e 100644
--- a/drivers/media/cec/platform/s5p/s5p_cec.c
+++ b/drivers/media/cec/platform/s5p/s5p_cec.c
@@ -35,10 +35,13 @@ MODULE_PARM_DESC(debug, "debug level (0-2)");
 
 static int s5p_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
+	int ret;
 	struct s5p_cec_dev *cec = cec_get_drvdata(adap);
 
 	if (enable) {
-		pm_runtime_get_sync(cec->dev);
+		ret = pm_runtime_resume_and_get(cec->dev);
+		if (ret < 0)
+			return ret;
 
 		s5p_cec_reset(cec);
 
-- 
2.30.2

[PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@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;
 
@@ -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

Re: [PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

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. 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

Re: [PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

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

Re: [PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

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:
>>

[PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
 drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..9d5841194f6b 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
 	struct gsc_dev *gsc = platform_get_drvdata(pdev);
 	int i;
 
-	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_resume_and_get(&pdev->dev);
 
 	gsc_unregister_m2m_device(gsc);
 	v4l2_device_unregister(&gsc->v4l2_dev);
@@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
 	for (i = 0; i < gsc->num_clocks; i++)
 		clk_disable_unprepare(gsc->clock[i]);
 
-	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
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

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

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/exynos-gsc/gsc-core.c | 3 +--
>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..9d5841194f6b 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>  	int i;
>  
> -	pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_resume_and_get(&pdev->dev);
>  
>  	gsc_unregister_m2m_device(gsc);
>  	v4l2_device_unregister(&gsc->v4l2_dev);
> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
>  	for (i = 0; i < gsc->num_clocks; i++)
>  		clk_disable_unprepare(gsc->clock[i]);
>  
> -	pm_runtime_put_noidle(&pdev->dev);

If we do this then the device usage count will not get decremented
after the pm_runtime_resume_and_get() call above and after driver
unload/load cycle it will not be possible to suspend the device.
I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
works better in that case.

Regards,
Sylwester

>  	pm_runtime_disable(&pdev->dev);
>  
>  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Tue, 27 Apr 2021 10:18:12 +0200
Sylwester Nawrocki <s.nawrocki@samsung.com> 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/exynos-gsc/gsc-core.c | 3 +--
> >  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > index 9f41c2e7097a..9d5841194f6b 100644
> > --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> >  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >  	int i;
> >  
> > -	pm_runtime_get_sync(&pdev->dev);
> > +	pm_runtime_resume_and_get(&pdev->dev);
> >  
> >  	gsc_unregister_m2m_device(gsc);
> >  	v4l2_device_unregister(&gsc->v4l2_dev);
> > @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> >  	for (i = 0; i < gsc->num_clocks; i++)
> >  		clk_disable_unprepare(gsc->clock[i]);
> >  
> > -	pm_runtime_put_noidle(&pdev->dev);  
> 
> If we do this then the device usage count will not get decremented
> after the pm_runtime_resume_and_get() call above and after driver
> unload/load cycle it will not be possible to suspend the device.
> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> works better in that case.

Good point.

Actually, I don't see any reason why to call a PM resume
function - either being pm_runtime_get_sync() or
pm_runtime_resume_and_get().

The code there could simply be:

    static int gsc_remove(struct platform_device *pdev)
    {
        struct gsc_dev *gsc = platform_get_drvdata(pdev);
        int i;

        gsc_unregister_m2m_device(gsc);
        v4l2_device_unregister(&gsc->v4l2_dev);

        vb2_dma_contig_clear_max_seg_size(&pdev->dev);
        for (i = 0; i < gsc->num_clocks; i++)
                clk_disable_unprepare(gsc->clock[i]);

        pm_runtime_disable(&pdev->dev);

        dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
        return 0;
    }

Eventually also adding:
	pm_runtime_suspended(&pdev->dev);

just after pm_runtime_disable().

Regards,
Mauro

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Tue, 27 Apr 2021 11:30:55 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Tue, 27 Apr 2021 10:18:12 +0200
> Sylwester Nawrocki <s.nawrocki@samsung.com> 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/exynos-gsc/gsc-core.c | 3 +--
> > >  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > index 9f41c2e7097a..9d5841194f6b 100644
> > > --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> > >  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >  	int i;
> > >  
> > > -	pm_runtime_get_sync(&pdev->dev);
> > > +	pm_runtime_resume_and_get(&pdev->dev);
> > >  
> > >  	gsc_unregister_m2m_device(gsc);
> > >  	v4l2_device_unregister(&gsc->v4l2_dev);
> > > @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> > >  	for (i = 0; i < gsc->num_clocks; i++)
> > >  		clk_disable_unprepare(gsc->clock[i]);
> > >  
> > > -	pm_runtime_put_noidle(&pdev->dev);    
> > 
> > If we do this then the device usage count will not get decremented
> > after the pm_runtime_resume_and_get() call above and after driver
> > unload/load cycle it will not be possible to suspend the device.
> > I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> > works better in that case.  
> 
> Good point.
> 
> Actually, I don't see any reason why to call a PM resume
> function - either being pm_runtime_get_sync() or
> pm_runtime_resume_and_get().
> 
> The code there could simply be:
> 
>     static int gsc_remove(struct platform_device *pdev)
>     {
>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
>         int i;
> 
>         gsc_unregister_m2m_device(gsc);
>         v4l2_device_unregister(&gsc->v4l2_dev);
> 
>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
>         for (i = 0; i < gsc->num_clocks; i++)
>                 clk_disable_unprepare(gsc->clock[i]);
> 
>         pm_runtime_disable(&pdev->dev);
> 
>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>         return 0;
>     }
> 
> Eventually also adding:
> 	pm_runtime_suspended(&pdev->dev);

In time: I actually meant:

	pm_runtime_set_suspended(&pdev->dev);

but after double-checking the PM runtime code, it sounds to me that
just calling pm_runtime_disable() would be enough. Not 100% sure
here. Btw, some media drivers call it after pm_runtime_disable(),
while others don't do.

Thanks,
Mauro

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Apr 2021 11:30:55 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
>> Em Tue, 27 Apr 2021 10:18:12 +0200
>> Sylwester Nawrocki <s.nawrocki@samsung.com> 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/exynos-gsc/gsc-core.c | 3 +--
>>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
>>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> index 9f41c2e7097a..9d5841194f6b 100644
>>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
>>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>>>>  	int i;
>>>>  
>>>> -	pm_runtime_get_sync(&pdev->dev);
>>>> +	pm_runtime_resume_and_get(&pdev->dev);
>>>>  
>>>>  	gsc_unregister_m2m_device(gsc);
>>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
>>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
>>>>  	for (i = 0; i < gsc->num_clocks; i++)
>>>>  		clk_disable_unprepare(gsc->clock[i]);
>>>>  
>>>> -	pm_runtime_put_noidle(&pdev->dev);    
>>>
>>> If we do this then the device usage count will not get decremented
>>> after the pm_runtime_resume_and_get() call above and after driver
>>> unload/load cycle it will not be possible to suspend the device.
>>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
>>> works better in that case.  
>>
>> Good point.
>>
>> Actually, I don't see any reason why to call a PM resume
>> function - either being pm_runtime_get_sync() or
>> pm_runtime_resume_and_get().
>>
>> The code there could simply be:
>>
>>     static int gsc_remove(struct platform_device *pdev)
>>     {
>>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
>>         int i;
>>
>>         gsc_unregister_m2m_device(gsc);
>>         v4l2_device_unregister(&gsc->v4l2_dev);
>>
>>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
>>         for (i = 0; i < gsc->num_clocks; i++)
>>                 clk_disable_unprepare(gsc->clock[i]);
>>
>>         pm_runtime_disable(&pdev->dev);
>>
>>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>>         return 0;
>>     }
>>
>> Eventually also adding:
>> 	pm_runtime_suspended(&pdev->dev);
> 
> In time: I actually meant:
> 
> 	pm_runtime_set_suspended(&pdev->dev);
> 
> but after double-checking the PM runtime code, it sounds to me that
> just calling pm_runtime_disable() would be enough. Not 100% sure
> here. Btw, some media drivers call it after pm_runtime_disable(),
> while others don't do.

I think if the device is brought into suspended state (e.g. by
disabling clocks as above) the pm_runtime_set_suspended() call
should be there. IOW a following sequence: 

	pm_runtime_disable(dev);
	if (!pm_runtime_status_suspended(dev))
		/* put device into suspended state (disable clocks, 
		  voltage regulators, assert GPIOs, etc. */
	pm_runtime_set_suspended(dev);

--
Regards,
Sylwester

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Tue, 27 Apr 2021 13:50:44 +0200
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:
> > Em Tue, 27 Apr 2021 11:30:55 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >   
> >> Em Tue, 27 Apr 2021 10:18:12 +0200
> >> Sylwester Nawrocki <s.nawrocki@samsung.com> 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/exynos-gsc/gsc-core.c | 3 +--
> >>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> >>>>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> index 9f41c2e7097a..9d5841194f6b 100644
> >>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> >>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >>>>  	int i;
> >>>>  
> >>>> -	pm_runtime_get_sync(&pdev->dev);
> >>>> +	pm_runtime_resume_and_get(&pdev->dev);
> >>>>  
> >>>>  	gsc_unregister_m2m_device(gsc);
> >>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
> >>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> >>>>  	for (i = 0; i < gsc->num_clocks; i++)
> >>>>  		clk_disable_unprepare(gsc->clock[i]);
> >>>>  
> >>>> -	pm_runtime_put_noidle(&pdev->dev);      
> >>>
> >>> If we do this then the device usage count will not get decremented
> >>> after the pm_runtime_resume_and_get() call above and after driver
> >>> unload/load cycle it will not be possible to suspend the device.
> >>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> >>> works better in that case.    
> >>
> >> Good point.
> >>
> >> Actually, I don't see any reason why to call a PM resume
> >> function - either being pm_runtime_get_sync() or
> >> pm_runtime_resume_and_get().
> >>
> >> The code there could simply be:
> >>
> >>     static int gsc_remove(struct platform_device *pdev)
> >>     {
> >>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >>         int i;
> >>
> >>         gsc_unregister_m2m_device(gsc);
> >>         v4l2_device_unregister(&gsc->v4l2_dev);
> >>
> >>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> >>         for (i = 0; i < gsc->num_clocks; i++)
> >>                 clk_disable_unprepare(gsc->clock[i]);
> >>
> >>         pm_runtime_disable(&pdev->dev);
> >>
> >>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> >>         return 0;
> >>     }
> >>
> >> Eventually also adding:
> >> 	pm_runtime_suspended(&pdev->dev);  
> > 
> > In time: I actually meant:
> > 
> > 	pm_runtime_set_suspended(&pdev->dev);
> > 
> > but after double-checking the PM runtime code, it sounds to me that
> > just calling pm_runtime_disable() would be enough. Not 100% sure
> > here. Btw, some media drivers call it after pm_runtime_disable(),
> > while others don't do.  
> 
> I think if the device is brought into suspended state (e.g. by
> disabling clocks as above) the pm_runtime_set_suspended() call
> should be there. IOW a following sequence: 
> 
> 	pm_runtime_disable(dev);
> 	if (!pm_runtime_status_suspended(dev))
> 		/* put device into suspended state (disable clocks, 
> 		  voltage regulators, assert GPIOs, etc. */
> 	pm_runtime_set_suspended(dev);

Not sure if this would work, as the clock framework would try
to do things like calling clk_pm_runtime_put().

Perhaps an alternative would be to just return an error if it
can't resume PM runtime, e. g.:

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..d47d02c75484 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1208,9 +1208,11 @@ static int gsc_probe(struct platform_device *pdev)
 static int gsc_remove(struct platform_device *pdev)
 {
        struct gsc_dev *gsc = platform_get_drvdata(pdev);
-       int i;
+       int ret, i;
 
-       pm_runtime_get_sync(&pdev->dev);
+       ret = pm_runtime_resume_and_get(&pdev->dev);
+       if (ret < 0)
+               return ret;
 
        gsc_unregister_m2m_device(gsc);
        v4l2_device_unregister(&gsc->v4l2_dev);


Thanks,
Mauro

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Wed, 28 Apr 2021 09:13:02 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Tue, 27 Apr 2021 13:50:44 +0200
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> 
> > On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 27 Apr 2021 11:30:55 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >     
> > >> Em Tue, 27 Apr 2021 10:18:12 +0200
> > >> Sylwester Nawrocki <s.nawrocki@samsung.com> 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/exynos-gsc/gsc-core.c | 3 +--
> > >>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> > >>>>  2 files changed, 2 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> index 9f41c2e7097a..9d5841194f6b 100644
> > >>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> > >>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >>>>  	int i;
> > >>>>  
> > >>>> -	pm_runtime_get_sync(&pdev->dev);
> > >>>> +	pm_runtime_resume_and_get(&pdev->dev);
> > >>>>  
> > >>>>  	gsc_unregister_m2m_device(gsc);
> > >>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
> > >>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> > >>>>  	for (i = 0; i < gsc->num_clocks; i++)
> > >>>>  		clk_disable_unprepare(gsc->clock[i]);
> > >>>>  
> > >>>> -	pm_runtime_put_noidle(&pdev->dev);        
> > >>>
> > >>> If we do this then the device usage count will not get decremented
> > >>> after the pm_runtime_resume_and_get() call above and after driver
> > >>> unload/load cycle it will not be possible to suspend the device.
> > >>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> > >>> works better in that case.      
> > >>
> > >> Good point.
> > >>
> > >> Actually, I don't see any reason why to call a PM resume
> > >> function - either being pm_runtime_get_sync() or
> > >> pm_runtime_resume_and_get().
> > >>
> > >> The code there could simply be:
> > >>
> > >>     static int gsc_remove(struct platform_device *pdev)
> > >>     {
> > >>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >>         int i;
> > >>
> > >>         gsc_unregister_m2m_device(gsc);
> > >>         v4l2_device_unregister(&gsc->v4l2_dev);
> > >>
> > >>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> > >>         for (i = 0; i < gsc->num_clocks; i++)
> > >>                 clk_disable_unprepare(gsc->clock[i]);
> > >>
> > >>         pm_runtime_disable(&pdev->dev);
> > >>
> > >>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> > >>         return 0;
> > >>     }
> > >>
> > >> Eventually also adding:
> > >> 	pm_runtime_suspended(&pdev->dev);    
> > > 
> > > In time: I actually meant:
> > > 
> > > 	pm_runtime_set_suspended(&pdev->dev);
> > > 
> > > but after double-checking the PM runtime code, it sounds to me that
> > > just calling pm_runtime_disable() would be enough. Not 100% sure
> > > here. Btw, some media drivers call it after pm_runtime_disable(),
> > > while others don't do.    
> > 
> > I think if the device is brought into suspended state (e.g. by
> > disabling clocks as above) the pm_runtime_set_suspended() call
> > should be there. IOW a following sequence: 
> > 
> > 	pm_runtime_disable(dev);
> > 	if (!pm_runtime_status_suspended(dev))
> > 		/* put device into suspended state (disable clocks, 
> > 		  voltage regulators, assert GPIOs, etc. */
> > 	pm_runtime_set_suspended(dev);  
> 
> Not sure if this would work, as the clock framework would try
> to do things like calling clk_pm_runtime_put().
> 
> Perhaps an alternative would be to just return an error if it
> can't resume PM runtime, e. g.:
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..d47d02c75484 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1208,9 +1208,11 @@ static int gsc_probe(struct platform_device *pdev)
>  static int gsc_remove(struct platform_device *pdev)
>  {
>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> -       int i;
> +       int ret, i;
>  
> -       pm_runtime_get_sync(&pdev->dev);
> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> +       if (ret < 0)
> +               return ret;

Nah, forget about that. Despite the platform driver having a return code,
support for it bogus:


static int platform_remove(struct device *_dev)
{
        struct platform_driver *drv = to_platform_driver(_dev->driver);
        struct platform_device *dev = to_platform_device(_dev);

        if (drv->remove) {
                int ret = drv->remove(dev);

                if (ret)
                        dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
        }
        dev_pm_domain_detach(_dev, true);

        return 0;
}

Thanks,
Mauro

Re: [PATCH 58/78] media: exynos-gsc: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

On 28.04.2021 09:17, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 09:13:02 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
>> Em Tue, 27 Apr 2021 13:50:44 +0200
>> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
>>
>>> On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:

>>> I think if the device is brought into suspended state (e.g. by
>>> disabling clocks as above) the pm_runtime_set_suspended() call
>>> should be there. IOW a following sequence:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	if (!pm_runtime_status_suspended(dev))
>>> 		/* put device into suspended state (disable clocks,
>>> 		  voltage regulators, assert GPIOs, etc. */
>>> 	pm_runtime_set_suspended(dev);
>>
>> Not sure if this would work, as the clock framework would try
>> to do things like calling clk_pm_runtime_put().

It's done in multiple drivers this way. clk_pm_runtime_put() operates
on different device - the clock supplier, not the consumer device.
We just need to disable runtime PM for GSC as the last step, to avoid
any possible v4l2 m2m device_run() call with runtime PM disabled.

>> Perhaps an alternative would be to just return an error if it
>> can't resume PM runtime, e. g.:
[...]
> Nah, forget about that. Despite the platform driver having a return code,
> support for it bogus:

Yes, we can't really stop remove() from driver level so as much complete
resource release is being done as possible.


Regards,
Sylwester

[PATCH 72/78] media: s3c-camif: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/s3c-camif/camif-capture.c | 5 ++---
 drivers/media/platform/s3c-camif/camif-core.c    | 5 +++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 9ca49af29542..01fa08065ebc 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -547,16 +547,15 @@ 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;
+		goto unlock;
 
 	ret = sensor_set_power(camif, 1);
 	if (!ret)
 		goto unlock;
 
 	pm_runtime_put(camif->dev);
-err_pm:
 	v4l2_fh_release(file);
 unlock:
 	mutex_unlock(&camif->lock);
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

Re: [PATCH 72/78] media: s3c-camif: use pm_runtime_resume_and_get()

[Sylwester Nawrocki]

Hi Mauro,

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/s3c-camif/camif-capture.c | 5 ++---
>   drivers/media/platform/s3c-camif/camif-core.c    | 5 +++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
> index 9ca49af29542..01fa08065ebc 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -547,16 +547,15 @@ 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;
> +		goto unlock;

I think we don't need that label change, fh still needs to be released
in case of an error.

>   	ret = sensor_set_power(camif, 1);
>   	if (!ret)
>   		goto unlock;
>   
>   	pm_runtime_put(camif->dev);
> -err_pm:
>   	v4l2_fh_release(file);
>   unlock:
>   	mutex_unlock(&camif->lock);

--
Regards,
Sylwester

Re: [PATCH 00/78] media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()

[Dan Carpenter]

There was a Smatch check for these bugs.  This was a good source of
recurring Reported-by tags for me.  ;)  Thanks for doing this.

regards,
dan carpenter