linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/25] Fix some PM runtime issues at the media subsystem
@ 2021-05-05  9:41 Mauro Carvalho Chehab
  2021-05-05  9:41 ` [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (25 more replies)
  0 siblings, 26 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Lad, Prabhakar,
	Paul J. Murphy, Andrzej Pietrasiewicz, Andy Gross,
	Bjorn Andersson, Chen-Yu Tsai, Daniele Alessandrelli,
	Ezequiel Garcia, Fabio Estevam, Jacek Anaszewski, Jernej Skrabec,
	Krzysztof Kozlowski, Marek Szyprowski, Matthias Brugger,
	Mauro Carvalho Chehab, Maxime Ripard, NXP Linux Team,
	Pengutronix Kernel Team, Philipp Zabel, Rui Miguel Silva,
	Sakari Ailus, Sascha Hauer, Shawn Guo, Stanimir Varbanov,
	Steve Longerbeam, Sylwester Nawrocki, linux-arm-kernel,
	linux-arm-msm, linux-kernel, linux-media, linux-mediatek,
	linux-renesas-soc, linux-rockchip, linux-samsung-soc,
	linux-staging, linux-sunxi

As part of an effort to cleanup pm_runtime*get* calls, I detected a number
of issues at the media subsystem.

Most of the patches here were submitted previously at:

	https://lore.kernel.org/linux-media/cover.1619621413.git.mchehab+huawei@kernel.org/

This series contain just the bug fixes and other related issues that are
present with the current code on media.

It address the points from the existing reviews. I also did my own
set of reviews, in order to avoid regressions.

Changes from v4 of the previous changeset:

- reworked i2c/css RPM get logic;
- dropped two patches that could cause regressions;
- am437x: keep using pm_runtime_get_sync on suspend/resume;
- atmel: fix the returned code and add a print on failures at start streaming;
- simplify some checks for return code > 0;
- mdk-vcodec: properly handle RPM errors at device on logic;
- venus: rework venus_sys_error_handler() logic;
- sti/delta: fix an issue at the error checking logic.

Mauro Carvalho Chehab (25):
  staging: media: rkvdec: fix pm_runtime_get_sync() usage count
  staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  media: venus: Rework error fail recover logic
  media: s5p_cec: decrement usage count if disabled
  media: i2c: ccs-core: return the right error code at suspend
  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: hantro: do a PM resume earlier
  media: marvel-ccic: fix some issues when getting pm_runtime
  media: mdk-mdp: fix pm_runtime_get_sync() usage count
  media: rcar_fdp1: simplify error check logic at fdp_open()
  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 logic
  media: s5p-jpeg: fix pm_runtime_get_sync() usage count
  media: sti/delta: use pm_runtime_resume_and_get()
  media: sunxi: fix pm_runtime_get_sync() usage count
  media: sti/bdisp: fix pm_runtime_get_sync() usage count
  media: exynos4-is: fix pm_runtime_get_sync() usage count
  media: exynos-gsc: fix pm_runtime_get_sync() usage count
  media: i2c: ccs-core: fix pm_runtime_get_sync() usage count

 drivers/media/cec/platform/s5p/s5p_cec.c      |  7 ++-
 drivers/media/i2c/ccs/ccs-core.c              | 41 ++++++++-----
 drivers/media/i2c/imx334.c                    |  7 ++-
 drivers/media/platform/am437x/am437x-vpfe.c   | 15 ++++-
 drivers/media/platform/atmel/atmel-isc-base.c | 30 +++++++---
 drivers/media/platform/atmel/atmel-isi.c      | 19 ++++--
 drivers/media/platform/exynos-gsc/gsc-core.c  | 11 ++--
 drivers/media/platform/exynos-gsc/gsc-m2m.c   |  4 +-
 .../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  |  5 +-
 drivers/media/platform/exynos4-is/media-dev.c |  9 +--
 drivers/media/platform/exynos4-is/mipi-csis.c | 10 ++--
 .../media/platform/marvell-ccic/mcam-core.c   |  9 ++-
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  6 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  |  4 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   |  8 ++-
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h   |  2 +-
 drivers/media/platform/qcom/venus/core.c      | 59 +++++++++++++++----
 drivers/media/platform/rcar_fdp1.c            | 28 ++++++---
 drivers/media/platform/renesas-ceu.c          |  4 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c   |  5 +-
 drivers/media/platform/sh_vou.c               |  6 +-
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c |  7 ++-
 drivers/media/platform/sti/delta/delta-v4l2.c |  8 +--
 .../sunxi/sun8i-rotate/sun8i_rotate.c         |  2 +-
 drivers/staging/media/hantro/hantro_drv.c     |  7 ++-
 drivers/staging/media/imx/imx7-mipi-csis.c    |  7 +--
 drivers/staging/media/rkvdec/rkvdec.c         |  2 +-
 32 files changed, 220 insertions(+), 127 deletions(-)

-- 
2.30.2



^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 12:23   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 02/25] staging: media: imx7-mipi-csis: " Mauro Carvalho Chehab
                   ` (24 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Ezequiel Garcia,
	Greg Kroah-Hartman, Mauro Carvalho Chehab, linux-kernel,
	linux-media, linux-rockchip, linux-staging

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, avoiding
a potential PM usage counter leak.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 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


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
  2021-05-05  9:41 ` [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 11:06   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 03/25] media: venus: Rework error fail recover logic Mauro Carvalho Chehab
                   ` (23 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Fabio Estevam,
	Greg Kroah-Hartman, Mauro Carvalho Chehab, NXP Linux Team,
	Pengutronix Kernel Team, Philipp Zabel, Rui Miguel Silva,
	Sascha Hauer, Shawn Guo, Steve Longerbeam, linux-arm-kernel,
	linux-kernel, linux-media, linux-staging

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, avoiding
a potential PM usage counter leak.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 025fdc488bd6..1dc680d94a46 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 
 		mipi_csis_clear_counters(state);
 
-		ret = pm_runtime_get_sync(&state->pdev->dev);
-		if (ret < 0) {
-			pm_runtime_put_noidle(&state->pdev->dev);
+		ret = pm_runtime_resume_and_get(&state->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
+
 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			goto done;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 03/25] media: venus: Rework error fail recover logic
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
  2021-05-05  9:41 ` [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
  2021-05-05  9:41 ` [PATCH 02/25] staging: media: imx7-mipi-csis: " Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 11:05   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 04/25] media: s5p_cec: decrement usage count if disabled Mauro Carvalho Chehab
                   ` (22 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
	Bjorn Andersson, Hans Verkuil, Mauro Carvalho Chehab,
	Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media

The Venus code has a sort of watchdog that attempts to recover
from IP errors, implemented as a delayed work job, which
calls venus_sys_error_handler().

Right now, it has several issues:

1. It assumes that PM runtime resume never fails

2. It internally runs two while() loops that also assume that
   PM runtime will never fail to go idle:

	while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
		msleep(10);

...

	while (core->pmdomains[0] && pm_runtime_active(core->pmdomains[0]))
		usleep_range(1000, 1500);

3. It uses an OR to merge all return codes and then report to the user

4. If the hardware never recovers, it keeps running on every 10ms,
   flooding the syslog with 2 messages (so, up to 200 messages
   per second).

Rework the code, in order to prevent that, by:

1. check the return code from PM runtime resume;
2. don't let the while() loops run forever;
3. store the failed event;
4. use warn ratelimited when it fails to recover.

Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/qcom/venus/core.c | 59 +++++++++++++++++++-----
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 54bac7ec14c5..4d0482743c0a 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -78,22 +78,32 @@ static const struct hfi_core_ops venus_core_ops = {
 	.event_notify = venus_event_notify,
 };
 
+#define RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS 10
+
 static void venus_sys_error_handler(struct work_struct *work)
 {
 	struct venus_core *core =
 			container_of(work, struct venus_core, work.work);
-	int ret = 0;
+	int ret, i, max_attempts = RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS;
+	bool failed = false;
+	const char *err_msg = "";
 
-	pm_runtime_get_sync(core->dev);
+	ret = pm_runtime_get_sync(core->dev);
+	if (ret < 0) {
+		err_msg = "resume runtime PM\n";
+		max_attempts = 0;
+		failed = true;
+	}
 
 	hfi_core_deinit(core, true);
 
-	dev_warn(core->dev, "system error has occurred, starting recovery!\n");
-
 	mutex_lock(&core->lock);
 
-	while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
+	for (i = 0; i < max_attempts; i++) {
+		if (!pm_runtime_active(core->dev_dec) && !pm_runtime_active(core->dev_enc))
+			break;
 		msleep(10);
+	}
 
 	venus_shutdown(core);
 
@@ -101,31 +111,56 @@ static void venus_sys_error_handler(struct work_struct *work)
 
 	pm_runtime_put_sync(core->dev);
 
-	while (core->pmdomains[0] && pm_runtime_active(core->pmdomains[0]))
+	for (i = 0; i < max_attempts; i++) {
+		if (!core->pmdomains[0] || !pm_runtime_active(core->pmdomains[0]))
+			break;
 		usleep_range(1000, 1500);
+	}
 
 	hfi_reinit(core);
 
-	pm_runtime_get_sync(core->dev);
+	ret = pm_runtime_get_sync(core->dev);
+	if (ret < 0) {
+		err_msg = "resume runtime PM\n";
+		max_attempts = 0;
+		failed = true;
+	}
 
-	ret |= venus_boot(core);
-	ret |= hfi_core_resume(core, true);
+	ret = venus_boot(core);
+	if (ret && !failed) {
+		err_msg = "boot Venus\n";
+		failed = true;
+	}
+
+	ret = hfi_core_resume(core, true);
+	if (ret && !failed) {
+		err_msg = "resume HFI\n";
+		failed = true;
+	}
 
 	enable_irq(core->irq);
 
 	mutex_unlock(&core->lock);
 
-	ret |= hfi_core_init(core);
+	ret = hfi_core_init(core);
+	if (ret && !failed) {
+		err_msg = "init HFI\n";
+		failed = true;
+	}
 
 	pm_runtime_put_sync(core->dev);
 
-	if (ret) {
+	if (failed) {
 		disable_irq_nosync(core->irq);
-		dev_warn(core->dev, "recovery failed (%d)\n", ret);
+		dev_warn_ratelimited(core->dev,
+				     "System error has occurred, recovery failed to %s\n",
+				     err_msg);
 		schedule_delayed_work(&core->work, msecs_to_jiffies(10));
 		return;
 	}
 
+	dev_warn(core->dev, "system error has occurred (recovered)\n");
+
 	mutex_lock(&core->lock);
 	core->sys_error = false;
 	mutex_unlock(&core->lock);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 04/25] media: s5p_cec: decrement usage count if disabled
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (2 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 03/25] media: venus: Rework error fail recover logic Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 12:24   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend Mauro Carvalho Chehab
                   ` (21 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Hans Verkuil,
	Kamil Debski, Marek Szyprowski, Mauro Carvalho Chehab,
	linux-kernel, linux-media, linux-samsung-soc, Sylwester Nawrocki

There's a bug at s5p_cec_adap_enable(): if called to
disable the device, it should call pm_runtime_put()
instead of pm_runtime_disable(), as the goal here is to
decrement the usage_count and not to disable PM runtime.

Reported-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Fixes: 1bcbf6f4b6b0 ("[media] cec: s5p-cec: Add s5p-cec driver")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/cec/platform/s5p/s5p_cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/platform/s5p/s5p_cec.c b/drivers/media/cec/platform/s5p/s5p_cec.c
index 2a3e7ffefe0a..3c7c4c3c798c 100644
--- a/drivers/media/cec/platform/s5p/s5p_cec.c
+++ b/drivers/media/cec/platform/s5p/s5p_cec.c
@@ -51,7 +51,7 @@ static int s5p_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	} else {
 		s5p_cec_mask_tx_interrupts(cec);
 		s5p_cec_mask_rx_interrupts(cec);
-		pm_runtime_disable(cec->dev);
+		pm_runtime_put(cec->dev);
 	}
 
 	return 0;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (3 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 04/25] media: s5p_cec: decrement usage count if disabled Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 12:24   ` Jonathan Cameron
  2021-05-05 12:51   ` Sakari Ailus
  2021-05-05  9:41 ` [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic Mauro Carvalho Chehab
                   ` (20 subsequent siblings)
  25 siblings, 2 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Mauro Carvalho Chehab, Sakari Ailus, linux-kernel, linux-media

If pm_runtime resume logic fails, return the error code
provided by it, instead of -EAGAIN, as, depending on what
caused it to fail, it may not be something that would be
recovered.

Fixes: cbba45d43631 ("[media] smiapp: Use runtime PM")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/ccs/ccs-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 9dc3f45da3dc..b05f409014b2 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -3093,7 +3093,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);
 
-		return -EAGAIN;
+		return rval;
 	}
 
 	if (sensor->streaming)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (4 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 11:10   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 07/25] media: exynos-gsc: don't resume at remove time Mauro Carvalho Chehab
                   ` (19 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Paul J. Murphy,
	Daniele Alessandrelli, Mauro Carvalho Chehab, linux-kernel,
	linux-media, Dan Carpenter

The PM runtime get logic is currently broken, as it checks if
ret is zero instead of checking if it is an error code,
as reported by Dan Carpenter.

While here, use the pm_runtime_resume_and_get() as added by:
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. As a bonus, such function
always return zero on success.

It should also be noticed that a fail of pm_runtime_get_sync() would
potentially result in a spurious runtime_suspend(), instead of
using pm_runtime_put_noidle().

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/imx334.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 047aa7658d21..23f28606e570 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	if (enable) {
-		ret = pm_runtime_get_sync(imx334->dev);
-		if (ret)
-			goto error_power_off;
+		ret = pm_runtime_resume_and_get(imx334->dev);
+		if (ret < 0)
+			goto error_unlock;
 
 		ret = imx334_start_streaming(imx334);
 		if (ret)
@@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
 
 error_power_off:
 	pm_runtime_put(imx334->dev);
+error_unlock:
 	mutex_unlock(&imx334->mutex);
 
 	return ret;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 07/25] media: exynos-gsc: don't resume at remove time
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (5 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 12:27   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 08/25] media: atmel: properly get pm_runtime Mauro Carvalho Chehab
                   ` (18 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Ezequiel Garcia,
	Hans Verkuil, Krzysztof Kozlowski, Mauro Carvalho Chehab,
	Sylwester Nawrocki, linux-arm-kernel, linux-kernel, linux-media,
	linux-samsung-soc

Calling pm_runtime_get_sync() at driver's removal time is not
needed, as this will resume PM runtime. Also, the PM runtime
code at pm_runtime_disable() already calls it, if it detects
the need.

So, change the logic in order to disable PM runtime earlier.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..f49f3322f835 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1210,18 +1210,19 @@ static int gsc_remove(struct platform_device *pdev)
 	struct gsc_dev *gsc = platform_get_drvdata(pdev);
 	int i;
 
-	pm_runtime_get_sync(&pdev->dev);
-
 	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_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		for (i = 0; i < gsc->num_clocks; i++)
+			clk_disable_unprepare(gsc->clock[i]);
+
+	pm_runtime_set_suspended(&pdev->dev);
+
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
 	return 0;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (6 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 07/25] media: exynos-gsc: don't resume at remove time Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 12:08   ` Jonathan Cameron
  2021-05-05  9:41 ` [PATCH 09/25] media: hantro: do a PM resume earlier Mauro Carvalho Chehab
                   ` (17 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Alexandre Belloni,
	Eugen Hristev, Ludovic Desroches, Mauro Carvalho Chehab,
	Nicolas Ferre, linux-arm-kernel, linux-kernel, linux-media

There are several issues in the way the atmel driver handles
pm_runtime_get_sync():

- it doesn't check return codes;
- it doesn't properly decrement the usage_count on all places;
- it starts streaming even if pm_runtime_get_sync() fails.
- while it tries to get pm_runtime at the clock enable logic,
  it doesn't check if the operation was suceeded.

Replace all occurrences of it to use the new kAPI:
pm_runtime_resume_and_get(), which ensures that, if the
return code is not negative, the usage_count was incremented.

With that, add additional checks when this is called, in order
to ensure that errors will be properly addressed.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
 drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index fe3ec8d0eaee..ce8e1351fa53 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
 static int isc_clk_prepare(struct clk_hw *hw)
 {
 	struct isc_clk *isc_clk = to_isc_clk(hw);
+	int ret;
 
-	if (isc_clk->id == ISC_ISPCK)
-		pm_runtime_get_sync(isc_clk->dev);
+	if (isc_clk->id == ISC_ISPCK) {
+		ret = pm_runtime_resume_and_get(isc_clk->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	return isc_wait_clk_stable(hw);
 }
@@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
 {
 	struct isc_clk *isc_clk = to_isc_clk(hw);
 	u32 status;
+	int ret;
 
-	if (isc_clk->id == ISC_ISPCK)
-		pm_runtime_get_sync(isc_clk->dev);
+	if (isc_clk->id == ISC_ISPCK) {
+		ret = pm_runtime_resume_and_get(isc_clk->dev);
+		if (ret < 0)
+			return 0;
+	}
 
 	regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
 
@@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err_start_stream;
 	}
 
-	pm_runtime_get_sync(isc->dev);
+	ret = pm_runtime_resume_and_get(isc->dev);
+	if (ret < 0) {
+		v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
+			 ret);
+		goto err_pm_get;
+	}
 
 	ret = isc_configure(isc);
 	if (unlikely(ret))
@@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 err_configure:
 	pm_runtime_put_sync(isc->dev);
-
+err_pm_get:
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
 
 err_start_stream:
@@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
 	u32 baysel;
 	unsigned long flags;
 	u32 min, max;
+	int ret;
 
 	/* streaming is not active anymore */
 	if (isc->stop)
@@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
 	ctrls->hist_id = hist_id;
 	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
 
-	pm_runtime_get_sync(isc->dev);
+	ret = pm_runtime_resume_and_get(isc->dev);
+	if (ret < 0)
+		return;
 
 	/*
 	 * only update if we have all the required histograms and controls
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index e392b3efe363..5b1dd358f2e6 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct frame_buffer *buf, *node;
 	int ret;
 
-	pm_runtime_get_sync(isi->dev);
+	ret = pm_runtime_resume_and_get(isi->dev);
+	if (ret < 0)
+		return ret;
 
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
@@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
 	return 0;
 }
 
-static void isi_camera_set_bus_param(struct atmel_isi *isi)
+static int isi_camera_set_bus_param(struct atmel_isi *isi)
 {
 	u32 cfg1 = 0;
+	int ret;
 
 	/* set bus param for ISI */
 	if (isi->pdata.hsync_act_low)
@@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
 	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
 
 	/* Enable PM and peripheral clock before operate isi registers */
-	pm_runtime_get_sync(isi->dev);
+	ret = pm_runtime_resume_and_get(isi->dev);
+	if (ret < 0)
+		return ret;
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 	isi_writel(isi, ISI_CFG1, cfg1);
 
 	pm_runtime_put(isi->dev);
+
+	return 0;
 }
 
 /* -----------------------------------------------------------------------*/
@@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 		dev_err(isi->dev, "No supported mediabus format found\n");
 		return ret;
 	}
-	isi_camera_set_bus_param(isi);
+	ret = isi_camera_set_bus_param(isi);
+	if (ret) {
+		dev_err(isi->dev, "Can't wake up device\n");
+		return ret;
+	}
 
 	ret = isi_set_default_fmt(isi);
 	if (ret) {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (7 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 08/25] media: atmel: properly get pm_runtime Mauro Carvalho Chehab
@ 2021-05-05  9:41 ` Mauro Carvalho Chehab
  2021-05-05 11:34   ` Jonathan Cameron
  2021-05-05 13:22   ` Ezequiel Garcia
  2021-05-05  9:42 ` [PATCH 10/25] media: marvel-ccic: fix some issues when getting pm_runtime Mauro Carvalho Chehab
                   ` (16 subsequent siblings)
  25 siblings, 2 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:41 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Ezequiel Garcia,
	Greg Kroah-Hartman, Hans Verkuil, Mauro Carvalho Chehab,
	Philipp Zabel, linux-kernel, linux-media, linux-rockchip,
	linux-staging

The device_run() first enables the clock and then
tries to resume PM runtime, checking for errors.

Well, if for some reason the pm_runtime can not resume,
it would be better to detect it beforehand.

So, change the order inside device_run().

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..4387edaa1d0d 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -152,13 +152,14 @@ static void device_run(void *priv)
 	src = hantro_get_src_buf(ctx);
 	dst = hantro_get_dst_buf(ctx);
 
-	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
-	if (ret)
-		goto err_cancel_job;
 	ret = pm_runtime_get_sync(ctx->dev->dev);
 	if (ret < 0)
 		goto err_cancel_job;
 
+	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
+	if (ret)
+		goto err_cancel_job;
+
 	v4l2_m2m_buf_copy_metadata(src, dst, true);
 
 	ctx->codec_ops->run(ctx);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 10/25] media: marvel-ccic: fix some issues when getting pm_runtime
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (8 preceding siblings ...)
  2021-05-05  9:41 ` [PATCH 09/25] media: hantro: do a PM resume earlier Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:42 ` [PATCH 11/25] media: mdk-mdp: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (15 subsequent siblings)
  25 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Gustavo A. R. Silva, Allen Pais, Chuhong Yuan, Ezequiel Garcia,
	Hans Verkuil, Helen Koike, Lubomir Rintel, Mauro Carvalho Chehab,
	Sakari Ailus, Vaibhav Gupta, linux-kernel, linux-media,
	Jonathan Cameron

Calling pm_runtime_get_sync() is bad, since even when it
returns an error, pm_runtime_put*() should be called.
So, use instead pm_runtime_resume_and_get().

While here, ensure that the error condition will be checked
during clock enable an media open() calls.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 141bf5d97a04..ea87110d9073 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -918,6 +918,7 @@ static int mclk_enable(struct clk_hw *hw)
 	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
 	int mclk_src;
 	int mclk_div;
+	int ret;
 
 	/*
 	 * Clock the sensor appropriately.  Controller clock should
@@ -931,7 +932,9 @@ static int mclk_enable(struct clk_hw *hw)
 		mclk_div = 2;
 	}
 
-	pm_runtime_get_sync(cam->dev);
+	ret = pm_runtime_resume_and_get(cam->dev);
+	if (ret < 0)
+		return ret;
 	clk_enable(cam->clk[0]);
 	mcam_reg_write(cam, REG_CLKCTRL, (mclk_src << 29) | mclk_div);
 	mcam_ctlr_power_up(cam);
@@ -1611,7 +1614,9 @@ static int mcam_v4l_open(struct file *filp)
 		ret = sensor_call(cam, core, s_power, 1);
 		if (ret)
 			goto out;
-		pm_runtime_get_sync(cam->dev);
+		ret = pm_runtime_resume_and_get(cam->dev);
+		if (ret < 0)
+			goto out;
 		__mcam_cam_reset(cam);
 		mcam_set_config_needed(cam, 1);
 	}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 11/25] media: mdk-mdp: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (9 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 10/25] media: marvel-ccic: fix some issues when getting pm_runtime Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:42 ` [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open() Mauro Carvalho Chehab
                   ` (14 subsequent siblings)
  25 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andrew-CT Chen,
	Houlong Wei, Matthias Brugger, Mauro Carvalho Chehab,
	Minghsiu Tsai, linux-arm-kernel, linux-kernel, linux-media,
	linux-mediatek, Jonathan Cameron

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, avoiding
a potential PM usage counter leak.

While here, fix the return contition of mtk_mdp_m2m_start_streaming(),
as it doesn't make any sense to return 0 if the PM runtime failed
to resume.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index ace4528cdc5e..f14779e7596e 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -391,12 +391,12 @@ static int mtk_mdp_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct mtk_mdp_ctx *ctx = q->drv_priv;
 	int ret;
 
-	ret = pm_runtime_get_sync(&ctx->mdp_dev->pdev->dev);
+	ret = pm_runtime_resume_and_get(&ctx->mdp_dev->pdev->dev);
 	if (ret < 0)
-		mtk_mdp_dbg(1, "[%d] pm_runtime_get_sync failed:%d",
+		mtk_mdp_dbg(1, "[%d] pm_runtime_resume_and_get failed:%d",
 			    ctx->id, ret);
 
-	return 0;
+	return ret;
 }
 
 static void *mtk_mdp_m2m_buf_remove(struct mtk_mdp_ctx *ctx,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open()
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (10 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 11/25] media: mdk-mdp: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:48   ` Sergei Shtylyov
  2021-05-05  9:42 ` [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (13 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Kieran Bingham,
	Mauro Carvalho Chehab, linux-kernel, linux-media,
	linux-renesas-soc

Avoid some code duplication by moving the common error path
logit at fdp_open().

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/rcar_fdp1.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
index 01c1fbb97bf6..d26413fa5205 100644
--- a/drivers/media/platform/rcar_fdp1.c
+++ b/drivers/media/platform/rcar_fdp1.c
@@ -2117,9 +2117,7 @@ static int fdp1_open(struct file *file)
 
 	if (ctx->hdl.error) {
 		ret = ctx->hdl.error;
-		v4l2_ctrl_handler_free(&ctx->hdl);
-		kfree(ctx);
-		goto done;
+		goto error_ctx;
 	}
 
 	ctx->fh.ctrl_handler = &ctx->hdl;
@@ -2133,10 +2131,7 @@ static int fdp1_open(struct file *file)
 
 	if (IS_ERR(ctx->fh.m2m_ctx)) {
 		ret = PTR_ERR(ctx->fh.m2m_ctx);
-
-		v4l2_ctrl_handler_free(&ctx->hdl);
-		kfree(ctx);
-		goto done;
+		goto error_ctx;
 	}
 
 	/* Perform any power management required */
@@ -2147,6 +2142,12 @@ static int fdp1_open(struct file *file)
 	dprintk(fdp1, "Created instance: %p, m2m_ctx: %p\n",
 		ctx, ctx->fh.m2m_ctx);
 
+	mutex_unlock(&fdp1->dev_mutex);
+	return 0;
+
+error_ctx:
+	v4l2_ctrl_handler_free(&ctx->hdl);
+	kfree(ctx);
 done:
 	mutex_unlock(&fdp1->dev_mutex);
 	return ret;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (11 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open() Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:31   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 14/25] media: renesas-ceu: Properly check for PM errors Mauro Carvalho Chehab
                   ` (12 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Kieran Bingham,
	Mauro Carvalho Chehab, linux-kernel, linux-media,
	linux-renesas-soc

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, avoiding
a potential PM usage counter leak.

Also, right now, the driver is ignoring any troubles when
trying to do PM resume. So, add the proper error handling
for the code.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/rcar_fdp1.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
index d26413fa5205..89aac60066d9 100644
--- a/drivers/media/platform/rcar_fdp1.c
+++ b/drivers/media/platform/rcar_fdp1.c
@@ -2135,7 +2135,9 @@ static int fdp1_open(struct file *file)
 	}
 
 	/* Perform any power management required */
-	pm_runtime_get_sync(fdp1->dev);
+	ret = pm_runtime_resume_and_get(fdp1->dev);
+	if (ret < 0)
+		goto error_pm;
 
 	v4l2_fh_add(&ctx->fh);
 
@@ -2145,6 +2147,8 @@ static int fdp1_open(struct file *file)
 	mutex_unlock(&fdp1->dev_mutex);
 	return 0;
 
+error_pm:
+       v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
 error_ctx:
 	v4l2_ctrl_handler_free(&ctx->hdl);
 	kfree(ctx);
@@ -2352,7 +2356,9 @@ static int fdp1_probe(struct platform_device *pdev)
 
 	/* Power up the cells to read HW */
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(fdp1->dev);
+	ret = pm_runtime_resume_and_get(fdp1->dev);
+	if (ret < 0)
+		goto disable_pm;
 
 	hw_version = fdp1_read(fdp1, FD1_IP_INTDATA);
 	switch (hw_version) {
@@ -2381,6 +2387,9 @@ static int fdp1_probe(struct platform_device *pdev)
 
 	return 0;
 
+disable_pm:
+	pm_runtime_disable(fdp1->dev);
+
 release_m2m:
 	v4l2_m2m_release(fdp1->m2m_dev);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 14/25] media: renesas-ceu: Properly check for PM errors
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (12 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:56   ` Jacopo Mondi
  2021-05-05  9:42 ` [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (11 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Jacopo Mondi,
	Mauro Carvalho Chehab, linux-kernel, linux-media,
	linux-renesas-soc, Jonathan Cameron

Right now, the driver just assumes that PM runtime resume
worked, but it may fail.

Well, the pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

So, using it is tricky. Let's 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")
and return an error if something bad happens.

This should ensure that the PM runtime usage_count will be
properly decremented if an error happens at open time.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/renesas-ceu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
index cd137101d41e..17f01b6e3fe0 100644
--- a/drivers/media/platform/renesas-ceu.c
+++ b/drivers/media/platform/renesas-ceu.c
@@ -1099,10 +1099,10 @@ static int ceu_open(struct file *file)
 
 	mutex_lock(&ceudev->mlock);
 	/* Causes soft-reset and sensor power on on first open */
-	pm_runtime_get_sync(ceudev->dev);
+	ret = pm_runtime_resume_and_get(ceudev->dev);
 	mutex_unlock(&ceudev->mlock);
 
-	return 0;
+	return ret;
 }
 
 static int ceu_release(struct file *file)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (13 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 14/25] media: renesas-ceu: Properly check for PM errors Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:31   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 16/25] media: am437x: " Mauro Carvalho Chehab
                   ` (10 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Hans Verkuil,
	Marek Szyprowski, Mauro Carvalho Chehab, linux-kernel,
	linux-media, linux-samsung-soc, Sylwester Nawrocki

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, avoiding
a potential PM usage counter leak.

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

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
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 3c7c4c3c798c..028a09a7531e 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


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 16/25] media: am437x: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (14 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:32   ` Jonathan Cameron
  2021-05-08 13:01   ` Lad, Prabhakar
  2021-05-05  9:42 ` [PATCH 17/25] media: sh_vou: " Mauro Carvalho Chehab
                   ` (9 subsequent siblings)
  25 siblings, 2 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Lad, Prabhakar,
	Mauro Carvalho Chehab, linux-kernel, linux-media

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, avoiding
a potential PM usage counter leak.

While here, ensure that the driver will check if PM runtime
resumed at vpfe_initialize_device().

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 6cdc77dda0e4..1c9cb9e05fdf 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe)
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(vpfe->pdev);
+	ret = pm_runtime_resume_and_get(vpfe->pdev);
+	if (ret < 0)
+		return ret;
 
 	vpfe_config_enable(&vpfe->ccdc, 1);
 
@@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 
 	/* for now just enable it here instead of waiting for the open */
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		vpfe_err(vpfe, "Unable to resume device.\n");
+		goto probe_out_v4l2_unregister;
+	}
 
 	vpfe_ccdc_config_defaults(ccdc);
 
@@ -2530,6 +2536,11 @@ static int vpfe_suspend(struct device *dev)
 
 	/* only do full suspend if streaming has started */
 	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
+		/*
+		 * ignore RPM resume errors here, as it is already too late.
+		 * A check like that should happen earlier, either at
+		 * open() or just before start streaming.
+		 */
 		pm_runtime_get_sync(dev);
 		vpfe_config_enable(ccdc, 1);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 17/25] media: sh_vou: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (15 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 16/25] media: am437x: " Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:42 ` [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic Mauro Carvalho Chehab
                   ` (8 subsequent siblings)
  25 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Gustavo A. R. Silva, Geert Uytterhoeven, Hans Verkuil,
	Jonathan Cameron, Mauro Carvalho Chehab, linux-kernel,
	linux-media

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, avoiding
a potential PM usage counter leak.

While here, check if the PM runtime error was caught at open time.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/sh_vou.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/sh_vou.c b/drivers/media/platform/sh_vou.c
index 4ac48441f22c..ca4310e26c49 100644
--- a/drivers/media/platform/sh_vou.c
+++ b/drivers/media/platform/sh_vou.c
@@ -1133,7 +1133,11 @@ static int sh_vou_open(struct file *file)
 	if (v4l2_fh_is_singular_file(file) &&
 	    vou_dev->status == SH_VOU_INITIALISING) {
 		/* First open */
-		pm_runtime_get_sync(vou_dev->v4l2_dev.dev);
+		err = pm_runtime_resume_and_get(vou_dev->v4l2_dev.dev);
+		if (err < 0) {
+			v4l2_fh_release(file);
+			goto done_open;
+		}
 		err = sh_vou_hw_init(vou_dev);
 		if (err < 0) {
 			pm_runtime_put(vou_dev->v4l2_dev.dev);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (16 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 17/25] media: sh_vou: " Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:32   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (7 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andrew-CT Chen,
	Matthias Brugger, Mauro Carvalho Chehab, Tiffany Lin,
	linux-arm-kernel, linux-kernel, linux-media, linux-mediatek

Currently, the driver just assumes that PM runtime logic
succeded resuming the device.

That may not be the case, as pm_runtime_get_sync()
can fail (but keeping the usage count incremented).

Replace the code to use pm_runtime_resume_and_get(),
and letting it return the error code.

This way, if mtk_vcodec_dec_pw_on() fails, the logic
under fops_vcodec_open() will do the right thing and
return an error, instead of just assuming that the
device is ready to be used.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 4 +++-
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c  | 8 +++++---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h  | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index 147dfef1638d..f87dc47d9e63 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -126,7 +126,9 @@ static int fops_vcodec_open(struct file *file)
 	mtk_vcodec_dec_set_default_params(ctx);
 
 	if (v4l2_fh_is_singular(&ctx->fh)) {
-		mtk_vcodec_dec_pw_on(&dev->pm);
+		ret = mtk_vcodec_dec_pw_on(&dev->pm);
+		if (ret < 0)
+			goto err_load_fw;
 		/*
 		 * Does nothing if firmware was already loaded.
 		 */
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..6038db96f71c 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -88,13 +88,15 @@ void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
 	put_device(dev->pm.larbvdec);
 }
 
-void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
+int 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);
+
+	return ret;
 }
 
 void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
index 872d8bf8cfaf..280aeaefdb65 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
@@ -12,7 +12,7 @@
 int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
 void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev);
 
-void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
+int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (17 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:33   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
                   ` (6 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Andrzej Pietrasiewicz, Jacek Anaszewski, Mauro Carvalho Chehab,
	Sylwester Nawrocki, linux-arm-kernel, linux-kernel, linux-media

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, avoiding
a potential PM usage counter leak.

As a plus, pm_runtime_resume_and_get() doesn't return
positive numbers, so the return code validation can
be removed.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 026111505f5a..d402e456f27d 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2566,11 +2566,8 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 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);
-
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(ctx->jpeg->dev);
 }
 
 static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get()
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (18 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:01   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 21/25] media: sunxi: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
                   ` (5 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Hugues Fruchet,
	Mauro Carvalho Chehab, linux-kernel, linux-media

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/sti/delta/delta-v4l2.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/sti/delta/delta-v4l2.c b/drivers/media/platform/sti/delta/delta-v4l2.c
index c691b3d81549..064a00a3084a 100644
--- a/drivers/media/platform/sti/delta/delta-v4l2.c
+++ b/drivers/media/platform/sti/delta/delta-v4l2.c
@@ -954,10 +954,8 @@ static void delta_run_work(struct work_struct *work)
 	/* enable the hardware */
 	if (!dec->pm) {
 		ret = delta_get_sync(ctx);
-		if (ret) {
-			delta_put_autosuspend(ctx);
+		if (ret)
 			goto err;
-		}
 	}
 
 	/* decode this access unit */
@@ -1277,9 +1275,9 @@ int delta_get_sync(struct delta_ctx *ctx)
 	int ret = 0;
 
 	/* enable the hardware */
-	ret = pm_runtime_get_sync(delta->dev);
+	ret = pm_runtime_resume_and_get(delta->dev);
 	if (ret < 0) {
-		dev_err(delta->dev, "%s pm_runtime_get_sync failed (%d)\n",
+		dev_err(delta->dev, "%s pm_runtime_resume_and_get failed (%d)\n",
 			__func__, ret);
 		return ret;
 	}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 21/25] media: sunxi: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (19 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05  9:42 ` [PATCH 22/25] media: sti/bdisp: " Mauro Carvalho Chehab
                   ` (4 subsequent siblings)
  25 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Chen-Yu Tsai,
	Jernej Skrabec, Mauro Carvalho Chehab, Maxime Ripard,
	linux-arm-kernel, linux-kernel, linux-media, linux-sunxi,
	Jonathan Cameron

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, avoiding
a potential PM usage counter leak.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c b/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
index 3f81dd17755c..fbcca59a0517 100644
--- a/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
+++ b/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
@@ -494,7 +494,7 @@ static int rotate_start_streaming(struct vb2_queue *vq, unsigned int count)
 		struct device *dev = ctx->dev->dev;
 		int ret;
 
-		ret = pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
 		if (ret < 0) {
 			dev_err(dev, "Failed to enable module\n");
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 22/25] media: sti/bdisp: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (20 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 21/25] media: sunxi: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:34   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 23/25] media: exynos4-is: " Mauro Carvalho Chehab
                   ` (3 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Fabien Dessenne,
	Mauro Carvalho Chehab, linux-kernel, linux-media

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

The bdisp_start_streaming() doesn't take it into account, which
would unbalance PM usage counter at bdisp_stop_streaming().

The logic at bdisp_probe() is correct, but the best is to use
the same call along the driver.

So, 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, avoiding
a potential PM usage counter leak.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
index 060ca85f64d5..85288da9d2ae 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
@@ -499,7 +499,7 @@ static int bdisp_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct bdisp_ctx *ctx = q->drv_priv;
 	struct vb2_v4l2_buffer *buf;
-	int ret = pm_runtime_get_sync(ctx->bdisp_dev->dev);
+	int ret = pm_runtime_resume_and_get(ctx->bdisp_dev->dev);
 
 	if (ret < 0) {
 		dev_err(ctx->bdisp_dev->dev, "failed to set runtime PM\n");
@@ -1364,10 +1364,10 @@ static int bdisp_probe(struct platform_device *pdev)
 
 	/* Power management */
 	pm_runtime_enable(dev);
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0) {
 		dev_err(dev, "failed to set PM\n");
-		goto err_pm;
+		goto err_remove;
 	}
 
 	/* Filters */
@@ -1395,6 +1395,7 @@ static int bdisp_probe(struct platform_device *pdev)
 	bdisp_hw_free_filters(bdisp->dev);
 err_pm:
 	pm_runtime_put(dev);
+err_remove:
 	bdisp_debugfs_remove(bdisp);
 	v4l2_device_unregister(&bdisp->v4l2_dev);
 err_clk:
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 23/25] media: exynos4-is: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (21 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 22/25] media: sti/bdisp: " Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:20   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 24/25] media: exynos-gsc: " Mauro Carvalho Chehab
                   ` (2 subsequent siblings)
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Krzysztof Kozlowski, Mauro Carvalho Chehab, Sylwester Nawrocki,
	linux-arm-kernel, linux-kernel, linux-media, linux-samsung-soc

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

On some places, this is ok, but on others the usage count
ended being unbalanced on failures.

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, avoiding
a potential PM usage counter leak.

As a bonus, such function always return zero on success. So,
some code can be simplified.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
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        |  4 ++--
 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       |  5 +----
 drivers/media/platform/exynos4-is/media-dev.c      |  9 +++------
 drivers/media/platform/exynos4-is/mipi-csis.c      | 10 ++++------
 8 files changed, 19 insertions(+), 30 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..1b24f5bfc4af 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -828,9 +828,9 @@ 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;
+		goto err_irq;
 
 	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
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..df8e2aa454d8 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -73,17 +73,14 @@ static void fimc_m2m_shutdown(struct fimc_ctx *ctx)
 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);
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(&ctx->fimc_dev->pdev->dev);
 }
 
 static void stop_streaming(struct vb2_queue *q)
 {
 	struct fimc_ctx *ctx = q->drv_priv;
 
-
 	fimc_m2m_shutdown(ctx);
 	fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
 	pm_runtime_put(&ctx->fimc_dev->pdev->dev);
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 13d192ba4aa6..e025178db06c 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,8 +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);
-	return ret < 0 ? ret : 0;
+	return pm_runtime_resume_and_get(camclk->fmd->pmf);
 }
 
 static void cam_clk_unprepare(struct clk_hw *hw)
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 1aac167abb17..ebf39c856894 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,11 +509,9 @@ 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);
-		if (ret && ret != 1) {
-			pm_runtime_put_noidle(&state->pdev->dev);
+		ret = pm_runtime_resume_and_get(&state->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
 	}
 
 	mutex_lock(&state->lock);
@@ -535,7 +533,7 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 	if (!enable)
 		pm_runtime_put(&state->pdev->dev);
 
-	return ret == 1 ? 0 : ret;
+	return ret;
 }
 
 static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 24/25] media: exynos-gsc: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (22 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 23/25] media: exynos4-is: " Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 12:34   ` Jonathan Cameron
  2021-05-05  9:42 ` [PATCH 25/25] media: i2c: ccs-core: " Mauro Carvalho Chehab
  2021-05-06 15:11 ` [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Ezequiel Garcia,
	Hans Verkuil, Krzysztof Kozlowski, Mauro Carvalho Chehab,
	Sylwester Nawrocki, linux-arm-kernel, linux-kernel, linux-media,
	linux-samsung-soc

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, avoiding
a potential PM usage counter leak.

As a bonus, as pm_runtime_get_sync() always return 0 on
success, the logic can be simplified.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 27a3c92c73bc..f1cf847d1cc2 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -56,10 +56,8 @@ static void __gsc_m2m_job_abort(struct gsc_ctx *ctx)
 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);
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
 }
 
 static void __gsc_m2m_cleanup_queue(struct gsc_ctx *ctx)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (23 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 24/25] media: exynos-gsc: " Mauro Carvalho Chehab
@ 2021-05-05  9:42 ` Mauro Carvalho Chehab
  2021-05-05 10:34   ` Sakari Ailus
  2021-05-06 15:11 ` [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
  25 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05  9:42 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Mauro Carvalho Chehab, Sakari Ailus, linux-kernel, linux-media

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

There is a bug at ccs_pm_get_init(): when this function returns
an error, the stream is not started, and RPM usage_count
should not be incremented. However, if the calls to
v4l2_ctrl_handler_setup() return errors, it will be kept
incremented.

At ccs_suspend() the best is to 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 automatically,
in the case of errors.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index b05f409014b2..04c3ab9e37b4 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
+	/*
+	 * It can't use pm_runtime_resume_and_get() here, as the driver
+	 * relies at the returned value to detect if the device was already
+	 * active or not.
+	 */
 	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
+	if (rval < 0)
+		goto error;
 
-		return rval;
-	} else if (!rval) {
-		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
-					       ctrl_handler);
-		if (rval)
-			return rval;
+	/* Device was already active, so don't set controls */
+	if (rval == 1)
+		return 0;
 
-		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
-	}
+	/* Restore V4L2 controls to the suspended device */
+	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
+	if (rval)
+		goto error;
 
+	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
+	if (rval)
+		goto error;
+
+	/* Keep PM runtime usage_count incremented on success */
 	return 0;
+error:
+	pm_runtime_put_noidle(&client->dev);
+	return rval;
 }
 
 static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
@@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 
-	rval = pm_runtime_get_sync(dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(dev);
-
+	rval = pm_runtime_resume_and_get(dev);
+	if (rval < 0)
 		return rval;
-	}
 
 	if (sensor->streaming)
 		ccs_stop_streaming(sensor);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 73+ messages in thread

* Re: [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open()
  2021-05-05  9:42 ` [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open() Mauro Carvalho Chehab
@ 2021-05-05  9:48   ` Sergei Shtylyov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergei Shtylyov @ 2021-05-05  9:48 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Kieran Bingham, Mauro Carvalho Chehab,
	linux-kernel, linux-media, linux-renesas-soc

Hello!

On 05.05.2021 12:42, Mauro Carvalho Chehab wrote:

> Avoid some code duplication by moving the common error path
> logit at fdp_open().

    Logic?

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 14/25] media: renesas-ceu: Properly check for PM errors
  2021-05-05  9:42 ` [PATCH 14/25] media: renesas-ceu: Properly check for PM errors Mauro Carvalho Chehab
@ 2021-05-05  9:56   ` Jacopo Mondi
  0 siblings, 0 replies; 73+ messages in thread
From: Jacopo Mondi @ 2021-05-05  9:56 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel,
	linux-media, linux-renesas-soc, Jonathan Cameron

Hi Mauro,

On Wed, May 05, 2021 at 11:42:04AM +0200, Mauro Carvalho Chehab wrote:
> Right now, the driver just assumes that PM runtime resume
> worked, but it may fail.
>
> Well, the pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
>
> So, using it is tricky. Let's 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")
> and return an error if something bad happens.
>
> This should ensure that the PM runtime usage_count will be
> properly decremented if an error happens at open time.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Looks good!
Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/renesas-ceu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> index cd137101d41e..17f01b6e3fe0 100644
> --- a/drivers/media/platform/renesas-ceu.c
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -1099,10 +1099,10 @@ static int ceu_open(struct file *file)
>
>  	mutex_lock(&ceudev->mlock);
>  	/* Causes soft-reset and sensor power on on first open */
> -	pm_runtime_get_sync(ceudev->dev);
> +	ret = pm_runtime_resume_and_get(ceudev->dev);
>  	mutex_unlock(&ceudev->mlock);
>
> -	return 0;
> +	return ret;
>  }
>
>  static int ceu_release(struct file *file)
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 25/25] media: i2c: ccs-core: " Mauro Carvalho Chehab
@ 2021-05-05 10:34   ` Sakari Ailus
  2021-05-05 10:57     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 73+ messages in thread
From: Sakari Ailus @ 2021-05-05 10:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel, linux-media

Hi Mauro,

Thanks for the patch.

On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote:
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> There is a bug at ccs_pm_get_init(): when this function returns
> an error, the stream is not started, and RPM usage_count
> should not be incremented. However, if the calls to
> v4l2_ctrl_handler_setup() return errors, it will be kept
> incremented.
> 
> At ccs_suspend() the best is to 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 automatically,
> in the case of errors.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Could you add Fixes: line and Cc: stable?

The patch that breaks this is 96e3a6b92f23a .

It would be better to fix the bug first so the patch to the stable trees
doesn't need special handling.

> ---
>  drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index b05f409014b2..04c3ab9e37b4 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>  	int rval;
>  
> +	/*
> +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> +	 * relies at the returned value to detect if the device was already
> +	 * active or not.
> +	 */
>  	rval = pm_runtime_get_sync(&client->dev);
> -	if (rval < 0) {
> -		pm_runtime_put_noidle(&client->dev);
> +	if (rval < 0)
> +		goto error;
>  
> -		return rval;
> -	} else if (!rval) {
> -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> -					       ctrl_handler);
> -		if (rval)
> -			return rval;
> +	/* Device was already active, so don't set controls */
> +	if (rval == 1)
> +		return 0;
>  
> -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> -	}
> +	/* Restore V4L2 controls to the suspended device */
> +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> +	if (rval)
> +		goto error;
>  
> +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> +	if (rval)
> +		goto error;
> +
> +	/* Keep PM runtime usage_count incremented on success */
>  	return 0;
> +error:
> +	pm_runtime_put_noidle(&client->dev);

This needs to be pm_runtime_put() as the device has been successfully.

> +	return rval;
>  }
>  
>  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
>  	bool streaming = sensor->streaming;
>  	int rval;
>  
> -	rval = pm_runtime_get_sync(dev);
> -	if (rval < 0) {
> -		pm_runtime_put_noidle(dev);
> -
> +	rval = pm_runtime_resume_and_get(dev);
> +	if (rval < 0)
>  		return rval;
> -	}
>  
>  	if (sensor->streaming)
>  		ccs_stop_streaming(sensor);

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 10:34   ` Sakari Ailus
@ 2021-05-05 10:57     ` Mauro Carvalho Chehab
  2021-05-05 10:58       ` Mauro Carvalho Chehab
  2021-05-05 11:02       ` Sakari Ailus
  0 siblings, 2 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 10:57 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel, linux-media

Em Wed, 5 May 2021 13:34:09 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thanks for the patch.
> 
> On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote:
> > The pm_runtime_get_sync() internally increments the
> > dev->power.usage_count without decrementing it, even on errors.
> > 
> > There is a bug at ccs_pm_get_init(): when this function returns
> > an error, the stream is not started, and RPM usage_count
> > should not be incremented. However, if the calls to
> > v4l2_ctrl_handler_setup() return errors, it will be kept
> > incremented.
> > 
> > At ccs_suspend() the best is to 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 automatically,
> > in the case of errors.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Could you add Fixes: line and Cc: stable?

Sure. See the fixes one enclosed.

> The patch that breaks this is 96e3a6b92f23a .
> 
> It would be better to fix the bug first so the patch to the stable trees
> doesn't need special handling.
> 
> > ---
> >  drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index b05f409014b2..04c3ab9e37b4 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> >  	int rval;
> >  
> > +	/*
> > +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> > +	 * relies at the returned value to detect if the device was already
> > +	 * active or not.
> > +	 */
> >  	rval = pm_runtime_get_sync(&client->dev);
> > -	if (rval < 0) {
> > -		pm_runtime_put_noidle(&client->dev);
> > +	if (rval < 0)
> > +		goto error;
> >  
> > -		return rval;
> > -	} else if (!rval) {
> > -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > -					       ctrl_handler);
> > -		if (rval)
> > -			return rval;
> > +	/* Device was already active, so don't set controls */
> > +	if (rval == 1)
> > +		return 0;
> >  
> > -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > -	}
> > +	/* Restore V4L2 controls to the suspended device */
> > +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> > +	if (rval)
> > +		goto error;
> >  
> > +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > +	if (rval)
> > +		goto error;
> > +
> > +	/* Keep PM runtime usage_count incremented on success */
> >  	return 0;
> > +error:
> > +	pm_runtime_put_noidle(&client->dev);  
> 
> This needs to be pm_runtime_put() as the device has been successfully.

Ok.

> 
> > +	return rval;
> >  }
> >  
> >  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> >  	bool streaming = sensor->streaming;
> >  	int rval;
> >  
> > -	rval = pm_runtime_get_sync(dev);
> > -	if (rval < 0) {
> > -		pm_runtime_put_noidle(dev);
> > -
> > +	rval = pm_runtime_resume_and_get(dev);
> > +	if (rval < 0)
> >  		return rval;
> > -	}
> >  
> >  	if (sensor->streaming)
> >  		ccs_stop_streaming(sensor);  
> 

Thanks,
Mauro

---

[PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

There is a bug at ccs_pm_get_init(): when this function returns
an error, the stream is not started, and RPM usage_count
should not be incremented. However, if the calls to
v4l2_ctrl_handler_setup() return errors, it will be kept
incremented.

At ccs_suspend() the best is to 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 automatically,
in the case of errors.

Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index b05f409014b2..5ea471fefa3a 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
+	/*
+	 * It can't use pm_runtime_resume_and_get() here, as the driver
+	 * relies at the returned value to detect if the device was already
+	 * active or not.
+	 */
 	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
+	if (rval < 0)
+		goto error;
 
-		return rval;
-	} else if (!rval) {
-		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
-					       ctrl_handler);
-		if (rval)
-			return rval;
+	/* Device was already active, so don't set controls */
+	if (rval == 1)
+		return 0;
 
-		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
-	}
+	/* Restore V4L2 controls to the suspended device */
+	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
+	if (rval)
+		goto error;
 
+	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
+	if (rval)
+		goto error;
+
+	/* Keep PM runtime usage_count incremented on success */
 	return 0;
+error:
+	pm_runtime_put(&client->dev);
+	return rval;
 }
 
 static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)





^ permalink raw reply related	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 10:57     ` Mauro Carvalho Chehab
@ 2021-05-05 10:58       ` Mauro Carvalho Chehab
  2021-05-05 12:35         ` Jonathan Cameron
  2021-05-05 11:02       ` Sakari Ailus
  1 sibling, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 10:58 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel, linux-media

Em Wed, 5 May 2021 12:57:00 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Wed, 5 May 2021 13:34:09 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the patch.
> > 
> > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote:
> > > The pm_runtime_get_sync() internally increments the
> > > dev->power.usage_count without decrementing it, even on errors.
> > > 
> > > There is a bug at ccs_pm_get_init(): when this function returns
> > > an error, the stream is not started, and RPM usage_count
> > > should not be incremented. However, if the calls to
> > > v4l2_ctrl_handler_setup() return errors, it will be kept
> > > incremented.
> > > 
> > > At ccs_suspend() the best is to 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 automatically,
> > > in the case of errors.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> > 
> > Could you add Fixes: line and Cc: stable?
> 
> Sure. See the fixes one enclosed.
> 
> > The patch that breaks this is 96e3a6b92f23a .
> > 
> > It would be better to fix the bug first so the patch to the stable trees
> > doesn't need special handling.
> > 
> > > ---
> > >  drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > index b05f409014b2..04c3ab9e37b4 100644
> > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > >  	int rval;
> > >  
> > > +	/*
> > > +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> > > +	 * relies at the returned value to detect if the device was already
> > > +	 * active or not.
> > > +	 */
> > >  	rval = pm_runtime_get_sync(&client->dev);
> > > -	if (rval < 0) {
> > > -		pm_runtime_put_noidle(&client->dev);
> > > +	if (rval < 0)
> > > +		goto error;
> > >  
> > > -		return rval;
> > > -	} else if (!rval) {
> > > -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > -					       ctrl_handler);
> > > -		if (rval)
> > > -			return rval;
> > > +	/* Device was already active, so don't set controls */
> > > +	if (rval == 1)
> > > +		return 0;
> > >  
> > > -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > -	}
> > > +	/* Restore V4L2 controls to the suspended device */
> > > +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> > > +	if (rval)
> > > +		goto error;
> > >  
> > > +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > +	if (rval)
> > > +		goto error;
> > > +
> > > +	/* Keep PM runtime usage_count incremented on success */
> > >  	return 0;
> > > +error:
> > > +	pm_runtime_put_noidle(&client->dev);  
> > 
> > This needs to be pm_runtime_put() as the device has been successfully.
> 
> Ok.
> 
> > 
> > > +	return rval;
> > >  }
> > >  
> > >  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > >  	bool streaming = sensor->streaming;
> > >  	int rval;
> > >  
> > > -	rval = pm_runtime_get_sync(dev);
> > > -	if (rval < 0) {
> > > -		pm_runtime_put_noidle(dev);
> > > -
> > > +	rval = pm_runtime_resume_and_get(dev);
> > > +	if (rval < 0)
> > >  		return rval;
> > > -	}
> > >  
> > >  	if (sensor->streaming)
> > >  		ccs_stop_streaming(sensor);  
> > 
> 
> Thanks,
> Mauro
> 
> ---
> 
> [PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> 
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> There is a bug at ccs_pm_get_init(): when this function returns
> an error, the stream is not started, and RPM usage_count
> should not be incremented. However, if the calls to
> v4l2_ctrl_handler_setup() return errors, it will be kept
> incremented.
> 
> At ccs_suspend() the best is to 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 automatically,
> in the case of errors.
> 
> Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index b05f409014b2..5ea471fefa3a 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>  	int rval;
>  
> +	/*
> +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> +	 * relies at the returned value to detect if the device was already
> +	 * active or not.
> +	 */
>  	rval = pm_runtime_get_sync(&client->dev);
> -	if (rval < 0) {
> -		pm_runtime_put_noidle(&client->dev);
> +	if (rval < 0)
> +		goto error;
>  
> -		return rval;
> -	} else if (!rval) {
> -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> -					       ctrl_handler);
> -		if (rval)
> -			return rval;
> +	/* Device was already active, so don't set controls */
> +	if (rval == 1)
> +		return 0;
>  
> -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> -	}
> +	/* Restore V4L2 controls to the suspended device */

In time: I'll fold this at the patch:

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 5ea471fefa3a..4a848ac2d2cd 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -1896 +1896 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
-       /* Restore V4L2 controls to the suspended device */
+       /* Restore V4L2 controls to the previously suspended device */

Regards,
Mauro

^ permalink raw reply related	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 10:57     ` Mauro Carvalho Chehab
  2021-05-05 10:58       ` Mauro Carvalho Chehab
@ 2021-05-05 11:02       ` Sakari Ailus
  1 sibling, 0 replies; 73+ messages in thread
From: Sakari Ailus @ 2021-05-05 11:02 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel, linux-media

Hi Mauro,

On Wed, May 05, 2021 at 12:57:00PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 5 May 2021 13:34:09 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the patch.
> > 
> > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote:
> > > The pm_runtime_get_sync() internally increments the
> > > dev->power.usage_count without decrementing it, even on errors.
> > > 
> > > There is a bug at ccs_pm_get_init(): when this function returns
> > > an error, the stream is not started, and RPM usage_count
> > > should not be incremented. However, if the calls to
> > > v4l2_ctrl_handler_setup() return errors, it will be kept
> > > incremented.
> > > 
> > > At ccs_suspend() the best is to 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 automatically,
> > > in the case of errors.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> > 
> > Could you add Fixes: line and Cc: stable?
> 
> Sure. See the fixes one enclosed.
> 
> > The patch that breaks this is 96e3a6b92f23a .
> > 
> > It would be better to fix the bug first so the patch to the stable trees
> > doesn't need special handling.

Please ignore this comment.

> > 
> > > ---
> > >  drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > index b05f409014b2..04c3ab9e37b4 100644
> > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > >  	int rval;
> > >  
> > > +	/*
> > > +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> > > +	 * relies at the returned value to detect if the device was already
> > > +	 * active or not.
> > > +	 */
> > >  	rval = pm_runtime_get_sync(&client->dev);
> > > -	if (rval < 0) {
> > > -		pm_runtime_put_noidle(&client->dev);
> > > +	if (rval < 0)
> > > +		goto error;
> > >  
> > > -		return rval;
> > > -	} else if (!rval) {
> > > -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > -					       ctrl_handler);
> > > -		if (rval)
> > > -			return rval;
> > > +	/* Device was already active, so don't set controls */
> > > +	if (rval == 1)
> > > +		return 0;
> > >  
> > > -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > -	}
> > > +	/* Restore V4L2 controls to the suspended device */
> > > +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> > > +	if (rval)
> > > +		goto error;
> > >  
> > > +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > +	if (rval)
> > > +		goto error;
> > > +
> > > +	/* Keep PM runtime usage_count incremented on success */
> > >  	return 0;
> > > +error:
> > > +	pm_runtime_put_noidle(&client->dev);  
> > 
> > This needs to be pm_runtime_put() as the device has been successfully.
> 
> Ok.
> 
> > 
> > > +	return rval;
> > >  }
> > >  
> > >  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > >  	bool streaming = sensor->streaming;
> > >  	int rval;
> > >  
> > > -	rval = pm_runtime_get_sync(dev);
> > > -	if (rval < 0) {
> > > -		pm_runtime_put_noidle(dev);
> > > -
> > > +	rval = pm_runtime_resume_and_get(dev);
> > > +	if (rval < 0)
> > >  		return rval;
> > > -	}
> > >  
> > >  	if (sensor->streaming)
> > >  		ccs_stop_streaming(sensor);  
> > 
> 
> Thanks,
> Mauro
> 
> ---
> 
> [PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> 
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> There is a bug at ccs_pm_get_init(): when this function returns
> an error, the stream is not started, and RPM usage_count
> should not be incremented. However, if the calls to
> v4l2_ctrl_handler_setup() return errors, it will be kept
> incremented.
> 
> At ccs_suspend() the best is to 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 automatically,
> in the case of errors.
> 
> Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index b05f409014b2..5ea471fefa3a 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>  	int rval;
>  
> +	/*
> +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> +	 * relies at the returned value to detect if the device was already
> +	 * active or not.
> +	 */
>  	rval = pm_runtime_get_sync(&client->dev);
> -	if (rval < 0) {
> -		pm_runtime_put_noidle(&client->dev);
> +	if (rval < 0)
> +		goto error;
>  
> -		return rval;
> -	} else if (!rval) {
> -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> -					       ctrl_handler);
> -		if (rval)
> -			return rval;
> +	/* Device was already active, so don't set controls */
> +	if (rval == 1)
> +		return 0;
>  
> -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> -	}
> +	/* Restore V4L2 controls to the suspended device */
> +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> +	if (rval)
> +		goto error;
>  
> +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> +	if (rval)
> +		goto error;
> +
> +	/* Keep PM runtime usage_count incremented on success */
>  	return 0;
> +error:
> +	pm_runtime_put(&client->dev);
> +	return rval;
>  }
>  
>  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> 
> 
> 
> 

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/25] media: venus: Rework error fail recover logic
  2021-05-05  9:41 ` [PATCH 03/25] media: venus: Rework error fail recover logic Mauro Carvalho Chehab
@ 2021-05-05 11:05   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 11:05 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson,
	Hans Verkuil, Mauro Carvalho Chehab, Stanimir Varbanov,
	linux-arm-msm, linux-kernel, linux-media

On Wed, 5 May 2021 11:41:53 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The Venus code has a sort of watchdog that attempts to recover
> from IP errors, implemented as a delayed work job, which
> calls venus_sys_error_handler().
> 
> Right now, it has several issues:
> 
> 1. It assumes that PM runtime resume never fails
> 
> 2. It internally runs two while() loops that also assume that
>    PM runtime will never fail to go idle:
> 
> 	while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
> 		msleep(10);
> 
> ...
> 
> 	while (core->pmdomains[0] && pm_runtime_active(core->pmdomains[0]))
> 		usleep_range(1000, 1500);
> 
> 3. It uses an OR to merge all return codes and then report to the user
> 
> 4. If the hardware never recovers, it keeps running on every 10ms,
>    flooding the syslog with 2 messages (so, up to 200 messages
>    per second).
> 
> Rework the code, in order to prevent that, by:
> 
> 1. check the return code from PM runtime resume;
> 2. don't let the while() loops run forever;
> 3. store the failed event;
> 4. use warn ratelimited when it fails to recover.
> 
> Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Trivial comments inline, otherwise based on no knowledge at all of the
actual hardware, the fix looks sane.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/qcom/venus/core.c | 59 +++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 54bac7ec14c5..4d0482743c0a 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -78,22 +78,32 @@ static const struct hfi_core_ops venus_core_ops = {
>  	.event_notify = venus_event_notify,
>  };
>  
> +#define RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS 10
> +
>  static void venus_sys_error_handler(struct work_struct *work)
>  {
>  	struct venus_core *core =
>  			container_of(work, struct venus_core, work.work);
> -	int ret = 0;
> +	int ret, i, max_attempts = RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS;
> +	bool failed = false;
> +	const char *err_msg = "";
>  
> -	pm_runtime_get_sync(core->dev);
> +	ret = pm_runtime_get_sync(core->dev);
> +	if (ret < 0) {
> +		err_msg = "resume runtime PM\n";

Will end up with two newlines I think as %s\n" later.

> +		max_attempts = 0;
> +		failed = true;
> +	}
>  
>  	hfi_core_deinit(core, true);
>  
> -	dev_warn(core->dev, "system error has occurred, starting recovery!\n");
> -
>  	mutex_lock(&core->lock);
>  
> -	while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
> +	for (i = 0; i < max_attempts; i++) {
> +		if (!pm_runtime_active(core->dev_dec) && !pm_runtime_active(core->dev_enc))
> +			break;
>  		msleep(10);
> +	}
>  
>  	venus_shutdown(core);
>  
> @@ -101,31 +111,56 @@ static void venus_sys_error_handler(struct work_struct *work)
>  
>  	pm_runtime_put_sync(core->dev);
>  
> -	while (core->pmdomains[0] && pm_runtime_active(core->pmdomains[0]))
> +	for (i = 0; i < max_attempts; i++) {
> +		if (!core->pmdomains[0] || !pm_runtime_active(core->pmdomains[0]))
> +			break;
>  		usleep_range(1000, 1500);
> +	}
>  
>  	hfi_reinit(core);
>  
> -	pm_runtime_get_sync(core->dev);
> +	ret = pm_runtime_get_sync(core->dev);
> +	if (ret < 0) {
> +		err_msg = "resume runtime PM\n";
> +		max_attempts = 0;

This is after the last use of max_attempts, so no point in setting it to zero.

> +		failed = true;
> +	}
>  
> -	ret |= venus_boot(core);
> -	ret |= hfi_core_resume(core, true);
> +	ret = venus_boot(core);
> +	if (ret && !failed) {
> +		err_msg = "boot Venus\n";
> +		failed = true;
> +	}
> +
> +	ret = hfi_core_resume(core, true);
> +	if (ret && !failed) {
> +		err_msg = "resume HFI\n";
> +		failed = true;
> +	}
>  
>  	enable_irq(core->irq);
>  
>  	mutex_unlock(&core->lock);
>  
> -	ret |= hfi_core_init(core);
> +	ret = hfi_core_init(core);
> +	if (ret && !failed) {
> +		err_msg = "init HFI\n";
> +		failed = true;
> +	}
>  
>  	pm_runtime_put_sync(core->dev);
>  
> -	if (ret) {
> +	if (failed) {
>  		disable_irq_nosync(core->irq);
> -		dev_warn(core->dev, "recovery failed (%d)\n", ret);
> +		dev_warn_ratelimited(core->dev,
> +				     "System error has occurred, recovery failed to %s\n",
> +				     err_msg);
>  		schedule_delayed_work(&core->work, msecs_to_jiffies(10));
>  		return;
>  	}
>  
> +	dev_warn(core->dev, "system error has occurred (recovered)\n");
> +
>  	mutex_lock(&core->lock);
>  	core->sys_error = false;
>  	mutex_unlock(&core->lock);


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 ` [PATCH 02/25] staging: media: imx7-mipi-csis: " Mauro Carvalho Chehab
@ 2021-05-05 11:06   ` Jonathan Cameron
  2021-05-05 11:17     ` Mauro Carvalho Chehab
  2021-05-05 13:56     ` Rui Miguel Silva
  0 siblings, 2 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 11:06 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Fabio Estevam, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, NXP Linux Team, Pengutronix Kernel Team,
	Philipp Zabel, Rui Miguel Silva, Sascha Hauer, Shawn Guo,
	Steve Longerbeam, linux-arm-kernel, linux-kernel, linux-media,
	linux-staging

On Wed, 5 May 2021 11:41:52 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Not a fix as far as I can see, just a cleanup - so perhaps not this set?

Jonathan


> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 025fdc488bd6..1dc680d94a46 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  
>  		mipi_csis_clear_counters(state);
>  
> -		ret = pm_runtime_get_sync(&state->pdev->dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(&state->pdev->dev);
> +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> +		if (ret < 0)
>  			return ret;
> -		}
> +
>  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
>  		if (ret < 0 && ret != -ENOIOCTLCMD)
>  			goto done;


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic
  2021-05-05  9:41 ` [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic Mauro Carvalho Chehab
@ 2021-05-05 11:10   ` Jonathan Cameron
  2021-05-05 11:24     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 11:10 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Paul J. Murphy, Daniele Alessandrelli,
	Mauro Carvalho Chehab, linux-kernel, linux-media, Dan Carpenter

On Wed, 5 May 2021 11:41:56 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The PM runtime get logic is currently broken, as it checks if
> ret is zero instead of checking if it is an error code,
> as reported by Dan Carpenter.
> 
> While here, use the pm_runtime_resume_and_get() as added by:
> 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. As a bonus, such function
> always return zero on success.
> 
> It should also be noticed that a fail of pm_runtime_get_sync() would
> potentially result in a spurious runtime_suspend(), instead of
> using pm_runtime_put_noidle().

Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
function change, you can stick with if (ret) and still be correct.

So only one of the two changes is needed to fix the bug.

J
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/i2c/imx334.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 047aa7658d21..23f28606e570 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	if (enable) {
> -		ret = pm_runtime_get_sync(imx334->dev);
> -		if (ret)
> -			goto error_power_off;
> +		ret = pm_runtime_resume_and_get(imx334->dev);
> +		if (ret < 0)
> +			goto error_unlock;
>  
>  		ret = imx334_start_streaming(imx334);
>  		if (ret)
> @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
>  
>  error_power_off:
>  	pm_runtime_put(imx334->dev);
> +error_unlock:
>  	mutex_unlock(&imx334->mutex);
>  
>  	return ret;


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  2021-05-05 11:06   ` Jonathan Cameron
@ 2021-05-05 11:17     ` Mauro Carvalho Chehab
  2021-05-05 13:56     ` Rui Miguel Silva
  1 sibling, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 11:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linuxarm, mauro.chehab, Fabio Estevam, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, NXP Linux Team, Pengutronix Kernel Team,
	Philipp Zabel, Rui Miguel Silva, Sascha Hauer, Shawn Guo,
	Steve Longerbeam, linux-arm-kernel, linux-kernel, linux-media,
	linux-staging

Em Wed, 5 May 2021 12:06:52 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?

Yeah, will move it to the non-error patch pile and change the comments.

> 
> Jonathan
> 
> 
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >  
> >  		mipi_csis_clear_counters(state);
> >  
> > -		ret = pm_runtime_get_sync(&state->pdev->dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(&state->pdev->dev);
> > +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > +		if (ret < 0)
> >  			return ret;
> > -		}
> > +
> >  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> >  		if (ret < 0 && ret != -ENOIOCTLCMD)
> >  			goto done;  
> 



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic
  2021-05-05 11:10   ` Jonathan Cameron
@ 2021-05-05 11:24     ` Mauro Carvalho Chehab
  2021-05-05 12:26       ` Jonathan Cameron
  0 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 11:24 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linuxarm, mauro.chehab, Paul J. Murphy, Daniele Alessandrelli,
	Mauro Carvalho Chehab, linux-kernel, linux-media, Dan Carpenter

Em Wed, 5 May 2021 12:10:40 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 5 May 2021 11:41:56 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > The PM runtime get logic is currently broken, as it checks if
> > ret is zero instead of checking if it is an error code,
> > as reported by Dan Carpenter.
> > 
> > While here, use the pm_runtime_resume_and_get() as added by:
> > 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. As a bonus, such function
> > always return zero on success.
> > 
> > It should also be noticed that a fail of pm_runtime_get_sync() would
> > potentially result in a spurious runtime_suspend(), instead of
> > using pm_runtime_put_noidle().  
> 
> Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
> function change, you can stick with if (ret) and still be correct.
> 
> So only one of the two changes is needed to fix the bug.

Yeah, I noticed ;-)

On media, almost all devices have I2C bus(es), and I2C send/receive functions
return positive values. So, a good practice is to check for errors with:

	if (ret < 0)

That's why I opted to keep both changes here ;-)

Regards,
Mauro

> 
> J
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/i2c/imx334.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 047aa7658d21..23f28606e570 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> >  	}
> >  
> >  	if (enable) {
> > -		ret = pm_runtime_get_sync(imx334->dev);
> > -		if (ret)
> > -			goto error_power_off;
> > +		ret = pm_runtime_resume_and_get(imx334->dev);
> > +		if (ret < 0)
> > +			goto error_unlock;
> >  
> >  		ret = imx334_start_streaming(imx334);
> >  		if (ret)
> > @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> >  
> >  error_power_off:
> >  	pm_runtime_put(imx334->dev);
> > +error_unlock:
> >  	mutex_unlock(&imx334->mutex);
> >  
> >  	return ret;  
> 



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05  9:41 ` [PATCH 09/25] media: hantro: do a PM resume earlier Mauro Carvalho Chehab
@ 2021-05-05 11:34   ` Jonathan Cameron
  2021-05-05 13:22   ` Ezequiel Garcia
  1 sibling, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 11:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Ezequiel Garcia, Greg Kroah-Hartman,
	Hans Verkuil, Mauro Carvalho Chehab, Philipp Zabel, linux-kernel,
	linux-media, linux-rockchip, linux-staging

On Wed, 5 May 2021 11:41:59 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The device_run() first enables the clock and then
> tries to resume PM runtime, checking for errors.
> 
> Well, if for some reason the pm_runtime can not resume,
> it would be better to detect it beforehand.
> 
> So, change the order inside device_run().
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Does this move not result in a potential call of clk_bulk_disable() for clocks
that aren't enabled?

> ---
>  drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..4387edaa1d0d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -152,13 +152,14 @@ static void device_run(void *priv)
>  	src = hantro_get_src_buf(ctx);
>  	dst = hantro_get_dst_buf(ctx);
>  
> -	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> -	if (ret)
> -		goto err_cancel_job;
>  	ret = pm_runtime_get_sync(ctx->dev->dev);
>  	if (ret < 0)
>  		goto err_cancel_job;
>  
> +	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> +	if (ret)
> +		goto err_cancel_job;
> +
>  	v4l2_m2m_buf_copy_metadata(src, dst, true);
>  
>  	ctx->codec_ops->run(ctx);


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get()
  2021-05-05  9:42 ` [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
@ 2021-05-05 12:01   ` Jonathan Cameron
  2021-05-05 12:33     ` Jonathan Cameron
  0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:01 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Hugues Fruchet, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 11:42:10 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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>
Trivial thing inline otherwise fine.

> ---
>  drivers/media/platform/sti/delta/delta-v4l2.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/sti/delta/delta-v4l2.c b/drivers/media/platform/sti/delta/delta-v4l2.c
> index c691b3d81549..064a00a3084a 100644
> --- a/drivers/media/platform/sti/delta/delta-v4l2.c
> +++ b/drivers/media/platform/sti/delta/delta-v4l2.c
> @@ -954,10 +954,8 @@ static void delta_run_work(struct work_struct *work)
>  	/* enable the hardware */
>  	if (!dec->pm) {
>  		ret = delta_get_sync(ctx);
> -		if (ret) {
> -			delta_put_autosuspend(ctx);
> +		if (ret)
>  			goto err;
> -		}
>  	}
>  
>  	/* decode this access unit */
> @@ -1277,9 +1275,9 @@ int delta_get_sync(struct delta_ctx *ctx)
>  	int ret = 0;

Loose the init

>  
>  	/* enable the hardware */
> -	ret = pm_runtime_get_sync(delta->dev);
> +	ret = pm_runtime_resume_and_get(delta->dev);
>  	if (ret < 0) {
> -		dev_err(delta->dev, "%s pm_runtime_get_sync failed (%d)\n",
> +		dev_err(delta->dev, "%s pm_runtime_resume_and_get failed (%d)\n",
>  			__func__, ret);
>  		return ret;
>  	}


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-05-05  9:41 ` [PATCH 08/25] media: atmel: properly get pm_runtime Mauro Carvalho Chehab
@ 2021-05-05 12:08   ` Jonathan Cameron
  2021-06-10  9:04     ` Eugen.Hristev
  0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:08 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Alexandre Belloni, Eugen Hristev,
	Ludovic Desroches, Mauro Carvalho Chehab, Nicolas Ferre,
	linux-arm-kernel, linux-kernel, linux-media

On Wed, 5 May 2021 11:41:58 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> There are several issues in the way the atmel driver handles
> pm_runtime_get_sync():
> 
> - it doesn't check return codes;
> - it doesn't properly decrement the usage_count on all places;
> - it starts streaming even if pm_runtime_get_sync() fails.
> - while it tries to get pm_runtime at the clock enable logic,
>   it doesn't check if the operation was suceeded.
> 
> Replace all occurrences of it to use the new kAPI:
> pm_runtime_resume_and_get(), which ensures that, if the
> return code is not negative, the usage_count was incremented.
> 
> With that, add additional checks when this is called, in order
> to ensure that errors will be properly addressed.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Hi Mauro, I don't know media enough to know what is the right answer
but in some of this series, a failure in
pm_runtime_resume_and_get() leads to a bunch of buffer cleanup
(patch 22 being an example) and in others return happens without doing
that cleanup.

It might be both are safe, or I'm missing something else, but I'm
certainly not confident enough to give any tags on this one as a result
of that mismatch.

> ---
>  drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
>  drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
>  2 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index fe3ec8d0eaee..ce8e1351fa53 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>  static int isc_clk_prepare(struct clk_hw *hw)
>  {
>  	struct isc_clk *isc_clk = to_isc_clk(hw);
> +	int ret;
>  
> -	if (isc_clk->id == ISC_ISPCK)
> -		pm_runtime_get_sync(isc_clk->dev);
> +	if (isc_clk->id == ISC_ISPCK) {
> +		ret = pm_runtime_resume_and_get(isc_clk->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return isc_wait_clk_stable(hw);
>  }
> @@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
>  {
>  	struct isc_clk *isc_clk = to_isc_clk(hw);
>  	u32 status;
> +	int ret;
>  
> -	if (isc_clk->id == ISC_ISPCK)
> -		pm_runtime_get_sync(isc_clk->dev);
> +	if (isc_clk->id == ISC_ISPCK) {
> +		ret = pm_runtime_resume_and_get(isc_clk->dev);
> +		if (ret < 0)
> +			return 0;
> +	}
>  
>  	regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
>  
> @@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		goto err_start_stream;
>  	}
>  
> -	pm_runtime_get_sync(isc->dev);
> +	ret = pm_runtime_resume_and_get(isc->dev);
> +	if (ret < 0) {
> +		v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
> +			 ret);
> +		goto err_pm_get;
> +	}
>  
>  	ret = isc_configure(isc);
>  	if (unlikely(ret))
> @@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  err_configure:
>  	pm_runtime_put_sync(isc->dev);
> -
> +err_pm_get:
>  	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>  
>  err_start_stream:
> @@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
>  	u32 baysel;
>  	unsigned long flags;
>  	u32 min, max;
> +	int ret;
>  
>  	/* streaming is not active anymore */
>  	if (isc->stop)
> @@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
>  	ctrls->hist_id = hist_id;
>  	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
>  
> -	pm_runtime_get_sync(isc->dev);
> +	ret = pm_runtime_resume_and_get(isc->dev);
> +	if (ret < 0)
> +		return;
>  
>  	/*
>  	 * only update if we have all the required histograms and controls
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index e392b3efe363..5b1dd358f2e6 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	struct frame_buffer *buf, *node;
>  	int ret;
>  
> -	pm_runtime_get_sync(isi->dev);
> +	ret = pm_runtime_resume_and_get(isi->dev);
> +	if (ret < 0)
> +		return ret;
This is the case I'm referring to above.

>  
>  	/* Enable stream on the sub device */
>  	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
> @@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static void isi_camera_set_bus_param(struct atmel_isi *isi)
> +static int isi_camera_set_bus_param(struct atmel_isi *isi)
>  {
>  	u32 cfg1 = 0;
> +	int ret;
>  
>  	/* set bus param for ISI */
>  	if (isi->pdata.hsync_act_low)
> @@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
>  	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>  
>  	/* Enable PM and peripheral clock before operate isi registers */
> -	pm_runtime_get_sync(isi->dev);
> +	ret = pm_runtime_resume_and_get(isi->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>  	isi_writel(isi, ISI_CFG1, cfg1);
>  
>  	pm_runtime_put(isi->dev);
> +
> +	return 0;
>  }
>  
>  /* -----------------------------------------------------------------------*/
> @@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>  		dev_err(isi->dev, "No supported mediabus format found\n");
>  		return ret;
>  	}
> -	isi_camera_set_bus_param(isi);
> +	ret = isi_camera_set_bus_param(isi);
> +	if (ret) {
> +		dev_err(isi->dev, "Can't wake up device\n");
> +		return ret;
> +	}
>  
>  	ret = isi_set_default_fmt(isi);
>  	if (ret) {


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 23/25] media: exynos4-is: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 23/25] media: exynos4-is: " Mauro Carvalho Chehab
@ 2021-05-05 12:20   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:20 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Krzysztof Kozlowski,
	Mauro Carvalho Chehab, Sylwester Nawrocki, linux-arm-kernel,
	linux-kernel, linux-media, linux-samsung-soc

On Wed, 5 May 2021 11:42:13 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> On some places, this is ok, but on others the usage count
> ended being unbalanced on failures.
> 
> 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, avoiding
> a potential PM usage counter leak.
> 
> As a bonus, such function always return zero on success. So,
> some code can be simplified.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Might be nice to call out the two odd cases below.

> diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
> index 1aac167abb17..ebf39c856894 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,11 +509,9 @@ 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);
> -		if (ret && ret != 1) {
Perhaps add something to the description on this less common case?

> -			pm_runtime_put_noidle(&state->pdev->dev);
> +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> +		if (ret < 0)
>  			return ret;
> -		}
>  	}
>  
>  	mutex_lock(&state->lock);
> @@ -535,7 +533,7 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (!enable)
>  		pm_runtime_put(&state->pdev->dev);
>  
> -	return ret == 1 ? 0 : ret;
> +	return ret;
>  }
>  
>  static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count
  2021-05-05  9:41 ` [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05 12:23   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:23 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Ezequiel Garcia, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, linux-kernel, linux-media, linux-rockchip,
	linux-staging

On Wed, 5 May 2021 11:41:51 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

(I'll do tags per patch as there are some open ones and it would be
odd to say - "excluding xxxx" in reply to the cover letter)

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


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 04/25] media: s5p_cec: decrement usage count if disabled
  2021-05-05  9:41 ` [PATCH 04/25] media: s5p_cec: decrement usage count if disabled Mauro Carvalho Chehab
@ 2021-05-05 12:24   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Hans Verkuil, Kamil Debski,
	Marek Szyprowski, Mauro Carvalho Chehab, linux-kernel,
	linux-media, linux-samsung-soc, Sylwester Nawrocki

On Wed, 5 May 2021 11:41:54 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> There's a bug at s5p_cec_adap_enable(): if called to
> disable the device, it should call pm_runtime_put()
> instead of pm_runtime_disable(), as the goal here is to
> decrement the usage_count and not to disable PM runtime.
> 
> Reported-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Fixes: 1bcbf6f4b6b0 ("[media] cec: s5p-cec: Add s5p-cec driver")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/cec/platform/s5p/s5p_cec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/platform/s5p/s5p_cec.c b/drivers/media/cec/platform/s5p/s5p_cec.c
> index 2a3e7ffefe0a..3c7c4c3c798c 100644
> --- a/drivers/media/cec/platform/s5p/s5p_cec.c
> +++ b/drivers/media/cec/platform/s5p/s5p_cec.c
> @@ -51,7 +51,7 @@ static int s5p_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  	} else {
>  		s5p_cec_mask_tx_interrupts(cec);
>  		s5p_cec_mask_rx_interrupts(cec);
> -		pm_runtime_disable(cec->dev);
> +		pm_runtime_put(cec->dev);
>  	}
>  
>  	return 0;


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend
  2021-05-05  9:41 ` [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend Mauro Carvalho Chehab
@ 2021-05-05 12:24   ` Jonathan Cameron
  2021-05-05 12:51   ` Sakari Ailus
  1 sibling, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Sakari Ailus,
	linux-kernel, linux-media

On Wed, 5 May 2021 11:41:55 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> If pm_runtime resume logic fails, return the error code
> provided by it, instead of -EAGAIN, as, depending on what
> caused it to fail, it may not be something that would be
> recovered.
> 
> Fixes: cbba45d43631 ("[media] smiapp: Use runtime PM")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/i2c/ccs/ccs-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 9dc3f45da3dc..b05f409014b2 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -3093,7 +3093,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
>  	if (rval < 0) {
>  		pm_runtime_put_noidle(dev);
>  
> -		return -EAGAIN;
> +		return rval;
>  	}
>  
>  	if (sensor->streaming)


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic
  2021-05-05 11:24     ` Mauro Carvalho Chehab
@ 2021-05-05 12:26       ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Paul J. Murphy, Daniele Alessandrelli,
	Mauro Carvalho Chehab, linux-kernel, linux-media, Dan Carpenter

On Wed, 5 May 2021 13:24:26 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 5 May 2021 12:10:40 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:
> 
> > On Wed, 5 May 2021 11:41:56 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > The PM runtime get logic is currently broken, as it checks if
> > > ret is zero instead of checking if it is an error code,
> > > as reported by Dan Carpenter.
> > > 
> > > While here, use the pm_runtime_resume_and_get() as added by:
> > > 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. As a bonus, such function
> > > always return zero on success.
> > > 
> > > It should also be noticed that a fail of pm_runtime_get_sync() would
> > > potentially result in a spurious runtime_suspend(), instead of
> > > using pm_runtime_put_noidle().    
> > 
> > Irony here is that pm_runtime_resume_and_get() returns <= 0 so with that
> > function change, you can stick with if (ret) and still be correct.
> > 
> > So only one of the two changes is needed to fix the bug.  
> 
> Yeah, I noticed ;-)
> 
> On media, almost all devices have I2C bus(es), and I2C send/receive functions
> return positive values. So, a good practice is to check for errors with:
> 
> 	if (ret < 0)
> 
> That's why I opted to keep both changes here ;-)

Fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Regards,
> Mauro
> 
> > 
> > J  
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  drivers/media/i2c/imx334.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 047aa7658d21..23f28606e570 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> > >  	}
> > >  
> > >  	if (enable) {
> > > -		ret = pm_runtime_get_sync(imx334->dev);
> > > -		if (ret)
> > > -			goto error_power_off;
> > > +		ret = pm_runtime_resume_and_get(imx334->dev);
> > > +		if (ret < 0)
> > > +			goto error_unlock;
> > >  
> > >  		ret = imx334_start_streaming(imx334);
> > >  		if (ret)
> > > @@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev *sd, int enable)
> > >  
> > >  error_power_off:
> > >  	pm_runtime_put(imx334->dev);
> > > +error_unlock:
> > >  	mutex_unlock(&imx334->mutex);
> > >  
> > >  	return ret;    
> >   
> 
> 
> 
> Thanks,
> Mauro


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/25] media: exynos-gsc: don't resume at remove time
  2021-05-05  9:41 ` [PATCH 07/25] media: exynos-gsc: don't resume at remove time Mauro Carvalho Chehab
@ 2021-05-05 12:27   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:27 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Ezequiel Garcia, Hans Verkuil,
	Krzysztof Kozlowski, Mauro Carvalho Chehab, Sylwester Nawrocki,
	linux-arm-kernel, linux-kernel, linux-media, linux-samsung-soc

On Wed, 5 May 2021 11:41:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Calling pm_runtime_get_sync() at driver's removal time is not
> needed, as this will resume PM runtime. Also, the PM runtime
> code at pm_runtime_disable() already calls it, if it detects
> the need.
> 
> So, change the logic in order to disable PM runtime earlier.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Seems sensible and a lot more along the lines of what I'd expect
to see than was originally here.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..f49f3322f835 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1210,18 +1210,19 @@ static int gsc_remove(struct platform_device *pdev)
>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>  	int i;
>  
> -	pm_runtime_get_sync(&pdev->dev);
> -
>  	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_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		for (i = 0; i < gsc->num_clocks; i++)
> +			clk_disable_unprepare(gsc->clock[i]);
> +
> +	pm_runtime_set_suspended(&pdev->dev);
> +
>  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>  	return 0;
>  }


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05 12:31   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:31 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Kieran Bingham, Mauro Carvalho Chehab,
	linux-kernel, linux-media, linux-renesas-soc

On Wed, 5 May 2021 11:42:03 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> Also, right now, the driver is ignoring any troubles when
> trying to do PM resume. So, add the proper error handling
> for the code.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/media/platform/rcar_fdp1.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
> index d26413fa5205..89aac60066d9 100644
> --- a/drivers/media/platform/rcar_fdp1.c
> +++ b/drivers/media/platform/rcar_fdp1.c
> @@ -2135,7 +2135,9 @@ static int fdp1_open(struct file *file)
>  	}
>  
>  	/* Perform any power management required */
> -	pm_runtime_get_sync(fdp1->dev);
> +	ret = pm_runtime_resume_and_get(fdp1->dev);
> +	if (ret < 0)
> +		goto error_pm;
>  
>  	v4l2_fh_add(&ctx->fh);
>  
> @@ -2145,6 +2147,8 @@ static int fdp1_open(struct file *file)
>  	mutex_unlock(&fdp1->dev_mutex);
>  	return 0;
>  
> +error_pm:
> +       v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  error_ctx:
>  	v4l2_ctrl_handler_free(&ctx->hdl);
>  	kfree(ctx);
> @@ -2352,7 +2356,9 @@ static int fdp1_probe(struct platform_device *pdev)
>  
>  	/* Power up the cells to read HW */
>  	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(fdp1->dev);
> +	ret = pm_runtime_resume_and_get(fdp1->dev);
> +	if (ret < 0)
> +		goto disable_pm;
>  
>  	hw_version = fdp1_read(fdp1, FD1_IP_INTDATA);
>  	switch (hw_version) {
> @@ -2381,6 +2387,9 @@ static int fdp1_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +disable_pm:
> +	pm_runtime_disable(fdp1->dev);
> +
>  release_m2m:
>  	v4l2_m2m_release(fdp1->m2m_dev);
>  


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05 12:31   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:31 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Hans Verkuil, Marek Szyprowski,
	Mauro Carvalho Chehab, linux-kernel, linux-media,
	linux-samsung-soc, Sylwester Nawrocki

On Wed, 5 May 2021 11:42:05 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> While here, check if the PM runtime error was caught at
> s5p_cec_adap_enable().
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 3c7c4c3c798c..028a09a7531e 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);
>  


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 16/25] media: am437x: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 16/25] media: am437x: " Mauro Carvalho Chehab
@ 2021-05-05 12:32   ` Jonathan Cameron
  2021-05-08 13:01   ` Lad, Prabhakar
  1 sibling, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:32 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Lad, Prabhakar, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 11:42:06 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> While here, ensure that the driver will check if PM runtime
> resumed at vpfe_initialize_device().
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 6cdc77dda0e4..1c9cb9e05fdf 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe)
>  	if (ret)
>  		return ret;
>  
> -	pm_runtime_get_sync(vpfe->pdev);
> +	ret = pm_runtime_resume_and_get(vpfe->pdev);
> +	if (ret < 0)
> +		return ret;
>  
>  	vpfe_config_enable(&vpfe->ccdc, 1);
>  
> @@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>  
>  	/* for now just enable it here instead of waiting for the open */
> -	pm_runtime_get_sync(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		vpfe_err(vpfe, "Unable to resume device.\n");
> +		goto probe_out_v4l2_unregister;
> +	}
>  
>  	vpfe_ccdc_config_defaults(ccdc);
>  
> @@ -2530,6 +2536,11 @@ static int vpfe_suspend(struct device *dev)
>  
>  	/* only do full suspend if streaming has started */
>  	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
> +		/*
> +		 * ignore RPM resume errors here, as it is already too late.
> +		 * A check like that should happen earlier, either at
> +		 * open() or just before start streaming.
> +		 */
>  		pm_runtime_get_sync(dev);
>  		vpfe_config_enable(ccdc, 1);
>  


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic
  2021-05-05  9:42 ` [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic Mauro Carvalho Chehab
@ 2021-05-05 12:32   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:32 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Andrew-CT Chen, Matthias Brugger,
	Mauro Carvalho Chehab, Tiffany Lin, linux-arm-kernel,
	linux-kernel, linux-media, linux-mediatek

On Wed,  5 May 2021 11:42:08 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Currently, the driver just assumes that PM runtime logic
> succeded resuming the device.
> 
> That may not be the case, as pm_runtime_get_sync()
> can fail (but keeping the usage count incremented).
> 
> Replace the code to use pm_runtime_resume_and_get(),
> and letting it return the error code.
> 
> This way, if mtk_vcodec_dec_pw_on() fails, the logic
> under fops_vcodec_open() will do the right thing and
> return an error, instead of just assuming that the
> device is ready to be used.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 4 +++-
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c  | 8 +++++---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h  | 2 +-
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> index 147dfef1638d..f87dc47d9e63 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> @@ -126,7 +126,9 @@ static int fops_vcodec_open(struct file *file)
>  	mtk_vcodec_dec_set_default_params(ctx);
>  
>  	if (v4l2_fh_is_singular(&ctx->fh)) {
> -		mtk_vcodec_dec_pw_on(&dev->pm);
> +		ret = mtk_vcodec_dec_pw_on(&dev->pm);
> +		if (ret < 0)
> +			goto err_load_fw;
>  		/*
>  		 * Does nothing if firmware was already loaded.
>  		 */
> 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..6038db96f71c 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> @@ -88,13 +88,15 @@ void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
>  	put_device(dev->pm.larbvdec);
>  }
>  
> -void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> +int 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);
> +
> +	return ret;
>  }
>  
>  void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> index 872d8bf8cfaf..280aeaefdb65 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> @@ -12,7 +12,7 @@
>  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
>  void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev);
>  
> -void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
> +int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm);


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
@ 2021-05-05 12:33   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:33 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Andrzej Pietrasiewicz, Jacek Anaszewski,
	Mauro Carvalho Chehab, Sylwester Nawrocki, linux-arm-kernel,
	linux-kernel, linux-media

On Wed, 5 May 2021 11:42:09 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> As a plus, pm_runtime_resume_and_get() doesn't return
> positive numbers, so the return code validation can
> be removed.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 026111505f5a..d402e456f27d 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2566,11 +2566,8 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>  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);
> -
> -	return ret > 0 ? 0 : ret;
> +	return pm_runtime_resume_and_get(ctx->jpeg->dev);
>  }
>  
>  static void s5p_jpeg_stop_streaming(struct vb2_queue *q)


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get()
  2021-05-05 12:01   ` Jonathan Cameron
@ 2021-05-05 12:33     ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:33 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Hugues Fruchet, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 13:01:58 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 5 May 2021 11:42:10 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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>  
> Trivial thing inline otherwise fine.
> 
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> > ---
> >  drivers/media/platform/sti/delta/delta-v4l2.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/sti/delta/delta-v4l2.c b/drivers/media/platform/sti/delta/delta-v4l2.c
> > index c691b3d81549..064a00a3084a 100644
> > --- a/drivers/media/platform/sti/delta/delta-v4l2.c
> > +++ b/drivers/media/platform/sti/delta/delta-v4l2.c
> > @@ -954,10 +954,8 @@ static void delta_run_work(struct work_struct *work)
> >  	/* enable the hardware */
> >  	if (!dec->pm) {
> >  		ret = delta_get_sync(ctx);
> > -		if (ret) {
> > -			delta_put_autosuspend(ctx);
> > +		if (ret)
> >  			goto err;
> > -		}
> >  	}
> >  
> >  	/* decode this access unit */
> > @@ -1277,9 +1275,9 @@ int delta_get_sync(struct delta_ctx *ctx)
> >  	int ret = 0;  
> 
> Loose the init
> 
> >  
> >  	/* enable the hardware */
> > -	ret = pm_runtime_get_sync(delta->dev);
> > +	ret = pm_runtime_resume_and_get(delta->dev);
> >  	if (ret < 0) {
> > -		dev_err(delta->dev, "%s pm_runtime_get_sync failed (%d)\n",
> > +		dev_err(delta->dev, "%s pm_runtime_resume_and_get failed (%d)\n",
> >  			__func__, ret);
> >  		return ret;
> >  	}  
> 


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 22/25] media: sti/bdisp: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 22/25] media: sti/bdisp: " Mauro Carvalho Chehab
@ 2021-05-05 12:34   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Fabien Dessenne, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 11:42:12 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> The bdisp_start_streaming() doesn't take it into account, which
> would unbalance PM usage counter at bdisp_stop_streaming().
> 
> The logic at bdisp_probe() is correct, but the best is to use
> the same call along the driver.
> 
> So, 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, avoiding
> a potential PM usage counter leak.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> index 060ca85f64d5..85288da9d2ae 100644
> --- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> +++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> @@ -499,7 +499,7 @@ static int bdisp_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>  	struct bdisp_ctx *ctx = q->drv_priv;
>  	struct vb2_v4l2_buffer *buf;
> -	int ret = pm_runtime_get_sync(ctx->bdisp_dev->dev);
> +	int ret = pm_runtime_resume_and_get(ctx->bdisp_dev->dev);
>  
>  	if (ret < 0) {
>  		dev_err(ctx->bdisp_dev->dev, "failed to set runtime PM\n");
> @@ -1364,10 +1364,10 @@ static int bdisp_probe(struct platform_device *pdev)
>  
>  	/* Power management */
>  	pm_runtime_enable(dev);
> -	ret = pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to set PM\n");
> -		goto err_pm;
> +		goto err_remove;
>  	}
>  
>  	/* Filters */
> @@ -1395,6 +1395,7 @@ static int bdisp_probe(struct platform_device *pdev)
>  	bdisp_hw_free_filters(bdisp->dev);
>  err_pm:
>  	pm_runtime_put(dev);
> +err_remove:
>  	bdisp_debugfs_remove(bdisp);
>  	v4l2_device_unregister(&bdisp->v4l2_dev);
>  err_clk:


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 24/25] media: exynos-gsc: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 24/25] media: exynos-gsc: " Mauro Carvalho Chehab
@ 2021-05-05 12:34   ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Ezequiel Garcia, Hans Verkuil,
	Krzysztof Kozlowski, Mauro Carvalho Chehab, Sylwester Nawrocki,
	linux-arm-kernel, linux-kernel, linux-media, linux-samsung-soc

On Wed, 5 May 2021 11:42:14 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
> 
> As a bonus, as pm_runtime_get_sync() always return 0 on
> success, the logic can be simplified.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 27a3c92c73bc..f1cf847d1cc2 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -56,10 +56,8 @@ static void __gsc_m2m_job_abort(struct gsc_ctx *ctx)
>  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);
> -	return ret > 0 ? 0 : ret;
> +	return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
>  }
>  
>  static void __gsc_m2m_cleanup_queue(struct gsc_ctx *ctx)


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 10:58       ` Mauro Carvalho Chehab
@ 2021-05-05 12:35         ` Jonathan Cameron
  2021-05-05 14:06           ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 12:35 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Sakari Ailus, linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 12:58:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 5 May 2021 12:57:00 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
> > Em Wed, 5 May 2021 13:34:09 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote:  
> > > > The pm_runtime_get_sync() internally increments the
> > > > dev->power.usage_count without decrementing it, even on errors.
> > > > 
> > > > There is a bug at ccs_pm_get_init(): when this function returns
> > > > an error, the stream is not started, and RPM usage_count
> > > > should not be incremented. However, if the calls to
> > > > v4l2_ctrl_handler_setup() return errors, it will be kept
> > > > incremented.
> > > > 
> > > > At ccs_suspend() the best is to 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 automatically,
> > > > in the case of errors.
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > > 
> > > Could you add Fixes: line and Cc: stable?  
> > 
> > Sure. See the fixes one enclosed.
> >   
> > > The patch that breaks this is 96e3a6b92f23a .
> > > 
> > > It would be better to fix the bug first so the patch to the stable trees
> > > doesn't need special handling.
> > >   
> > > > ---
> > > >  drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------
> > > >  1 file changed, 24 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > index b05f409014b2..04c3ab9e37b4 100644
> > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > > >  	int rval;
> > > >  
> > > > +	/*
> > > > +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> > > > +	 * relies at the returned value to detect if the device was already
> > > > +	 * active or not.
> > > > +	 */
> > > >  	rval = pm_runtime_get_sync(&client->dev);
> > > > -	if (rval < 0) {
> > > > -		pm_runtime_put_noidle(&client->dev);
> > > > +	if (rval < 0)
> > > > +		goto error;
> > > >  
> > > > -		return rval;
> > > > -	} else if (!rval) {
> > > > -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > > -					       ctrl_handler);
> > > > -		if (rval)
> > > > -			return rval;
> > > > +	/* Device was already active, so don't set controls */
> > > > +	if (rval == 1)
> > > > +		return 0;
> > > >  
> > > > -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > > -	}
> > > > +	/* Restore V4L2 controls to the suspended device */
> > > > +	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
> > > > +	if (rval)
> > > > +		goto error;
> > > >  
> > > > +	rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > > > +	if (rval)
> > > > +		goto error;
> > > > +
> > > > +	/* Keep PM runtime usage_count incremented on success */
> > > >  	return 0;
> > > > +error:
> > > > +	pm_runtime_put_noidle(&client->dev);    
> > > 
> > > This needs to be pm_runtime_put() as the device has been successfully.  
> > 
> > Ok.
> >   
> > >   
> > > > +	return rval;
> > > >  }
> > > >  
> > > >  static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
> > > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > > >  	bool streaming = sensor->streaming;
> > > >  	int rval;
> > > >  
> > > > -	rval = pm_runtime_get_sync(dev);
> > > > -	if (rval < 0) {
> > > > -		pm_runtime_put_noidle(dev);
> > > > -
> > > > +	rval = pm_runtime_resume_and_get(dev);
> > > > +	if (rval < 0)
> > > >  		return rval;
> > > > -	}
> > > >  
> > > >  	if (sensor->streaming)
> > > >  		ccs_stop_streaming(sensor);    
> > >   
> > 
> > Thanks,
> > Mauro
> > 
> > ---
> > 
> > [PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> > 
> > The pm_runtime_get_sync() internally increments the
> > dev->power.usage_count without decrementing it, even on errors.
> > 
> > There is a bug at ccs_pm_get_init(): when this function returns
> > an error, the stream is not started, and RPM usage_count
> > should not be incremented. However, if the calls to
> > v4l2_ctrl_handler_setup() return errors, it will be kept
> > incremented.
> > 
> > At ccs_suspend() the best is to 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 automatically,
> > in the case of errors.
> > 
> > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> > 
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index b05f409014b2..5ea471fefa3a 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> >  	int rval;
> >  
> > +	/*
> > +	 * It can't use pm_runtime_resume_and_get() here, as the driver
> > +	 * relies at the returned value to detect if the device was already
> > +	 * active or not.
> > +	 */
> >  	rval = pm_runtime_get_sync(&client->dev);
> > -	if (rval < 0) {
> > -		pm_runtime_put_noidle(&client->dev);
> > +	if (rval < 0)
> > +		goto error;
> >  
> > -		return rval;
> > -	} else if (!rval) {
> > -		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > -					       ctrl_handler);
> > -		if (rval)
> > -			return rval;
> > +	/* Device was already active, so don't set controls */
> > +	if (rval == 1)
> > +		return 0;
> >  
> > -		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> > -	}
> > +	/* Restore V4L2 controls to the suspended device */  
> 
> In time: I'll fold this at the patch:
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 5ea471fefa3a..4a848ac2d2cd 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -1896 +1896 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> -       /* Restore V4L2 controls to the suspended device */
> +       /* Restore V4L2 controls to the previously suspended device */
> 
> Regards,
> Mauro


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend
  2021-05-05  9:41 ` [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend Mauro Carvalho Chehab
  2021-05-05 12:24   ` Jonathan Cameron
@ 2021-05-05 12:51   ` Sakari Ailus
  1 sibling, 0 replies; 73+ messages in thread
From: Sakari Ailus @ 2021-05-05 12:51 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, linux-kernel, linux-media

On Wed, May 05, 2021 at 11:41:55AM +0200, Mauro Carvalho Chehab wrote:
> If pm_runtime resume logic fails, return the error code
> provided by it, instead of -EAGAIN, as, depending on what
> caused it to fail, it may not be something that would be
> recovered.
> 
> Fixes: cbba45d43631 ("[media] smiapp: Use runtime PM")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05  9:41 ` [PATCH 09/25] media: hantro: do a PM resume earlier Mauro Carvalho Chehab
  2021-05-05 11:34   ` Jonathan Cameron
@ 2021-05-05 13:22   ` Ezequiel Garcia
  2021-05-05 13:46     ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 73+ messages in thread
From: Ezequiel Garcia @ 2021-05-05 13:22 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Hans Verkuil,
	Mauro Carvalho Chehab, Philipp Zabel, linux-kernel, linux-media,
	linux-rockchip, linux-staging

Hi Mauro,

Thanks for working on this.

On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> The device_run() first enables the clock and then
> tries to resume PM runtime, checking for errors.
> 
> Well, if for some reason the pm_runtime can not resume,
> it would be better to detect it beforehand.
> 
> So, change the order inside device_run().
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

It seems this is wrong now, as this series doesn't have

https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/

I don't fully understand why all the back and forth
happening on this series, but the former Hantro patches
looked good (despite perhaps unclear commit messages).

Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":

  media: hantro: use pm_runtime_resume_and_get()
  media: hantro: do a PM resume earlier

?

Thanks,
Ezequiel

>  drivers/staging/media/hantro/hantro_drv.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..4387edaa1d0d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -152,13 +152,14 @@ static void device_run(void *priv)
>         src = hantro_get_src_buf(ctx);
>         dst = hantro_get_dst_buf(ctx);
>  
> -       ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> -       if (ret)
> -               goto err_cancel_job;
>         ret = pm_runtime_get_sync(ctx->dev->dev);
>         if (ret < 0)
>                 goto err_cancel_job;
>  
> +       ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> +       if (ret)
> +               goto err_cancel_job;
> +
>         v4l2_m2m_buf_copy_metadata(src, dst, true);
>  
>         ctx->codec_ops->run(ctx);



^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05 13:22   ` Ezequiel Garcia
@ 2021-05-05 13:46     ` Mauro Carvalho Chehab
  2021-05-05 14:01       ` Ezequiel Garcia
  0 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 13:46 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Hans Verkuil,
	Mauro Carvalho Chehab, Philipp Zabel, linux-kernel, linux-media,
	linux-rockchip, linux-staging

Em Wed, 05 May 2021 10:22:03 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> Hi Mauro,
> 
> Thanks for working on this.
> 
> On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> > The device_run() first enables the clock and then
> > tries to resume PM runtime, checking for errors.
> > 
> > Well, if for some reason the pm_runtime can not resume,
> > it would be better to detect it beforehand.
> > 
> > So, change the order inside device_run().
> > 
> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> It seems this is wrong now, as this series doesn't have
> 
> https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
> 
> I don't fully understand why all the back and forth
> happening on this series, but the former Hantro patches
> looked good (despite perhaps unclear commit messages).

There was a request to break the original /79 series into smaller ones,
to make easier for reviewers. So, I opted to split it into (probably)
3 series:

1. Fixes (this series);
2. "use pm_runtime_resume_and_get" for the I2C drivers;
3. "use pm_runtime_resume_and_get" for remaining ones.

Before flooding everybody's email's with series (2) and (3), better
to focus at the fixes first. I'll probably send the other two series
by tomorrow.

> Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
> 
>   media: hantro: use pm_runtime_resume_and_get()
>   media: hantro: do a PM resume earlier

The problem is that pm_runtime_resume_and_get() was added only
recently (Kernel v5.10). 

So, I opted to place the fix patches before the changes, as this
way, most (all?) patches can be easily be backported to legacy Kernels
as needed.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  2021-05-05 11:06   ` Jonathan Cameron
  2021-05-05 11:17     ` Mauro Carvalho Chehab
@ 2021-05-05 13:56     ` Rui Miguel Silva
  2021-05-05 14:23       ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 73+ messages in thread
From: Rui Miguel Silva @ 2021-05-05 13:56 UTC (permalink / raw)
  To: Jonathan Cameron, Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Fabio Estevam, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, NXP Linux Team, Pengutronix Kernel Team,
	Philipp Zabel, Sascha Hauer, Shawn Guo, Steve Longerbeam,
	linux-arm-kernel, linux-kernel, linux-media, linux-staging

Hi,
On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:

> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?

yes, the original changelog of this patch, that I acked,  made it
clear it was a cleanup:

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

This one above is new, but I saw Mauro is going change it.

------
Cheers,
     Rui

>
> Jonathan
>
>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >  
> >  		mipi_csis_clear_counters(state);
> >  
> > -		ret = pm_runtime_get_sync(&state->pdev->dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(&state->pdev->dev);
> > +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > +		if (ret < 0)
> >  			return ret;
> > -		}
> > +
> >  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> >  		if (ret < 0 && ret != -ENOIOCTLCMD)
> >  			goto done;




^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05 13:46     ` Mauro Carvalho Chehab
@ 2021-05-05 14:01       ` Ezequiel Garcia
  2021-05-05 14:15         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 73+ messages in thread
From: Ezequiel Garcia @ 2021-05-05 14:01 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Hans Verkuil,
	Mauro Carvalho Chehab, Philipp Zabel, linux-kernel, linux-media,
	linux-rockchip, linux-staging

On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 05 May 2021 10:22:03 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for working on this.
> > 
> > On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:
> > > The device_run() first enables the clock and then
> > > tries to resume PM runtime, checking for errors.
> > > 
> > > Well, if for some reason the pm_runtime can not resume,
> > > it would be better to detect it beforehand.
> > > 
> > > So, change the order inside device_run().
> > > 
> > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> > 
> > It seems this is wrong now, as this series doesn't have
> > 
> > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
> > 
> > I don't fully understand why all the back and forth
> > happening on this series, but the former Hantro patches
> > looked good (despite perhaps unclear commit messages).
> 
> There was a request to break the original /79 series into smaller ones,
> to make easier for reviewers. So, I opted to split it into (probably)
> 3 series:
> 
> 1. Fixes (this series);
> 2. "use pm_runtime_resume_and_get" for the I2C drivers;
> 3. "use pm_runtime_resume_and_get" for remaining ones.
> 
> Before flooding everybody's email's with series (2) and (3), better
> to focus at the fixes first. I'll probably send the other two series
> by tomorrow.
> 
> > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
> > 
> >   media: hantro: use pm_runtime_resume_and_get()
> >   media: hantro: do a PM resume earlier
> 
> The problem is that pm_runtime_resume_and_get() was added only
> recently (Kernel v5.10). 
> 
> So, I opted to place the fix patches before the changes, as this
> way, most (all?) patches can be easily be backported to legacy Kernels
> as needed.
> 

Got it.

Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync.

And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get.

Thanks,
Ezequiel



^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 12:35         ` Jonathan Cameron
@ 2021-05-05 14:06           ` Mauro Carvalho Chehab
  2021-05-05 16:36             ` Jonathan Cameron
  0 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 14:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sakari Ailus, linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	linux-kernel, linux-media

Hi Jonathan,

Em Wed, 5 May 2021 13:35:48 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> > > [PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> > > 
> > > The pm_runtime_get_sync() internally increments the
> > > dev->power.usage_count without decrementing it, even on errors.
> > > 
> > > There is a bug at ccs_pm_get_init(): when this function returns
> > > an error, the stream is not started, and RPM usage_count
> > > should not be incremented. However, if the calls to
> > > v4l2_ctrl_handler_setup() return errors, it will be kept
> > > incremented.
> > > 
> > > At ccs_suspend() the best is to 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 automatically,
> > > in the case of errors.
> > > 
> > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Per Sakari's request (for practical reasons on backporting and
c/c stable), this was split into two separate patches, one
fixing the issues, and a separate trivial one with just the
pm_runtime_resume_and_get(). I'm adding your RB on both.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 09/25] media: hantro: do a PM resume earlier
  2021-05-05 14:01       ` Ezequiel Garcia
@ 2021-05-05 14:15         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 14:15 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Hans Verkuil,
	Mauro Carvalho Chehab, Philipp Zabel, linux-kernel, linux-media,
	linux-rockchip, linux-staging

Em Wed, 05 May 2021 11:01:35 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 05 May 2021 10:22:03 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Thanks for working on this.
> > > 
> > > On Wed, 2021-05-05 at 11:41 +0200, Mauro Carvalho Chehab wrote:  
> > > > The device_run() first enables the clock and then
> > > > tries to resume PM runtime, checking for errors.
> > > > 
> > > > Well, if for some reason the pm_runtime can not resume,
> > > > it would be better to detect it beforehand.
> > > > 
> > > > So, change the order inside device_run().
> > > > 
> > > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > > 
> > > It seems this is wrong now, as this series doesn't have
> > > 
> > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/
> > > 
> > > I don't fully understand why all the back and forth
> > > happening on this series, but the former Hantro patches
> > > looked good (despite perhaps unclear commit messages).  
> > 
> > There was a request to break the original /79 series into smaller ones,
> > to make easier for reviewers. So, I opted to split it into (probably)
> > 3 series:
> > 
> > 1. Fixes (this series);
> > 2. "use pm_runtime_resume_and_get" for the I2C drivers;
> > 3. "use pm_runtime_resume_and_get" for remaining ones.
> > 
> > Before flooding everybody's email's with series (2) and (3), better
> > to focus at the fixes first. I'll probably send the other two series
> > by tomorrow.
> >   
> > > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem":
> > > 
> > >   media: hantro: use pm_runtime_resume_and_get()
> > >   media: hantro: do a PM resume earlier  
> > 
> > The problem is that pm_runtime_resume_and_get() was added only
> > recently (Kernel v5.10). 
> > 
> > So, I opted to place the fix patches before the changes, as this
> > way, most (all?) patches can be easily be backported to legacy Kernels
> > as needed.
> >   
> 
> Got it.
> 
> Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
> and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync.
> 
> And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get.

Works for me. So, the fixes patch will be the enclosed one, right?

Btw, I agree with Jonathan that the best would be to also move this:

	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);

out of hantro_job_finish_no_pm(), as, when an error happens at
device_run(), the clock lines won't be enabled at the first place.

> Thanks,
> Ezequiel

Thanks,
Mauro

[PATCH] media: hantro: do a PM resume earlier

The device_run() first enables the clock and then
tries to resume PM runtime, checking for errors.

Well, if for some reason the pm_runtime can not resume,
it would be better to detect it beforehand.

So, change the order inside device_run().

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..bdb57fb56f47 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
 	return hantro_get_dec_buf_addr(ctx, buf);
 }
 
-static void hantro_job_finish(struct hantro_dev *vpu,
-			      struct hantro_ctx *ctx,
-			      enum vb2_buffer_state result)
+static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
+				    struct hantro_ctx *ctx,
+				    enum vb2_buffer_state result)
 {
 	struct vb2_v4l2_buffer *src, *dst;
 
-	pm_runtime_mark_last_busy(vpu->dev);
-	pm_runtime_put_autosuspend(vpu->dev);
 	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
 
 	src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
@@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
 					 result);
 }
 
+static void hantro_job_finish(struct hantro_dev *vpu,
+			      struct hantro_ctx *ctx,
+			      enum vb2_buffer_state result)
+{
+	pm_runtime_mark_last_busy(vpu->dev);
+	pm_runtime_put_autosuspend(vpu->dev);
+
+	hantro_job_finish_no_pm(vpu, ctx, result);
+}
+
 void hantro_irq_done(struct hantro_dev *vpu,
 		     enum vb2_buffer_state result)
 {
@@ -152,12 +160,15 @@ static void device_run(void *priv)
 	src = hantro_get_src_buf(ctx);
 	dst = hantro_get_dst_buf(ctx);
 
+	ret = pm_runtime_get_sync(ctx->dev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(ctx->dev->dev);
+		goto err_cancel_job;
+	}
+
 	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
 	if (ret)
 		goto err_cancel_job;
-	ret = pm_runtime_get_sync(ctx->dev->dev);
-	if (ret < 0)
-		goto err_cancel_job;
 
 	v4l2_m2m_buf_copy_metadata(src, dst, true);
 
@@ -165,7 +176,7 @@ static void device_run(void *priv)
 	return;
 
 err_cancel_job:
-	hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
 }
 
 static struct v4l2_m2m_ops vpu_m2m_ops = {



^ permalink raw reply related	[flat|nested] 73+ messages in thread

* Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  2021-05-05 13:56     ` Rui Miguel Silva
@ 2021-05-05 14:23       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 14:23 UTC (permalink / raw)
  To: Rui Miguel Silva
  Cc: Jonathan Cameron, linuxarm, mauro.chehab, Fabio Estevam,
	Greg Kroah-Hartman, Mauro Carvalho Chehab, NXP Linux Team,
	Pengutronix Kernel Team, Philipp Zabel, Sascha Hauer, Shawn Guo,
	Steve Longerbeam, linux-arm-kernel, linux-kernel, linux-media,
	linux-staging

Em Wed, 05 May 2021 14:56:40 +0100
"Rui Miguel Silva" <rmfrfs@gmail.com> escreveu:

> Hi,
> On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:
> 
> > On Wed, 5 May 2021 11:41:52 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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, avoiding
> > > a potential PM usage counter leak.
> > > 
> > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> >
> > Not a fix as far as I can see, just a cleanup - so perhaps not this set?  
> 
> yes, the original changelog of this patch, that I acked,  made it
> clear it was a cleanup:
> 
> "
> 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.
> "
> 
> This one above is new, but I saw Mauro is going change it.

Yes, I'll change the subject/description to the
"use pm_runtime_resume_and_get()" one on this patch, as there's
no issue to be fixed here, just a cleanup ;-)

Sorry for the mess. I did lots of rebase on ~80 patch series
over the last couple of days, based on the reviews (and my own
internal reviews)...

See, the current patchset has ~80 patches with ~30% contained
fixes. It shows that writing a balanced PM runtime code is not
so trivial ;-)

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 25/25] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
  2021-05-05 14:06           ` Mauro Carvalho Chehab
@ 2021-05-05 16:36             ` Jonathan Cameron
  0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Cameron @ 2021-05-05 16:36 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Sakari Ailus, linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	linux-kernel, linux-media

On Wed, 5 May 2021 16:06:45 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Hi Jonathan,
> 
> Em Wed, 5 May 2021 13:35:48 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:
> 
> > > > [PATCH] media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> > > > 
> > > > The pm_runtime_get_sync() internally increments the
> > > > dev->power.usage_count without decrementing it, even on errors.
> > > > 
> > > > There is a bug at ccs_pm_get_init(): when this function returns
> > > > an error, the stream is not started, and RPM usage_count
> > > > should not be incremented. However, if the calls to
> > > > v4l2_ctrl_handler_setup() return errors, it will be kept
> > > > incremented.
> > > > 
> > > > At ccs_suspend() the best is to 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 automatically,
> > > > in the case of errors.
> > > > 
> > > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Per Sakari's request (for practical reasons on backporting and
> c/c stable), this was split into two separate patches, one
> fixing the issues, and a separate trivial one with just the
> pm_runtime_resume_and_get(). I'm adding your RB on both.
Makes sense.

Jonathan

> 
> Thanks,
> Mauro


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 00/25] Fix some PM runtime issues at the media subsystem
  2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
                   ` (24 preceding siblings ...)
  2021-05-05  9:42 ` [PATCH 25/25] media: i2c: ccs-core: " Mauro Carvalho Chehab
@ 2021-05-06 15:11 ` Mauro Carvalho Chehab
  25 siblings, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-06 15:11 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Lad, Prabhakar, Paul J. Murphy,
	Andrzej Pietrasiewicz, Andy Gross, Bjorn Andersson, Chen-Yu Tsai,
	Daniele Alessandrelli, Ezequiel Garcia, Fabio Estevam,
	Jacek Anaszewski, Jernej Skrabec, Krzysztof Kozlowski,
	Marek Szyprowski, Matthias Brugger, Mauro Carvalho Chehab,
	Maxime Ripard, NXP Linux Team, Pengutronix Kernel Team,
	Philipp Zabel, Rui Miguel Silva, Sakari Ailus, Sascha Hauer,
	Shawn Guo, Stanimir Varbanov, Steve Longerbeam,
	Sylwester Nawrocki, linux-arm-kernel, linux-arm-msm,
	linux-kernel, linux-media, linux-mediatek, linux-renesas-soc,
	linux-rockchip, linux-samsung-soc, linux-staging, linux-sunxi

Em Wed,  5 May 2021 11:41:50 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> As part of an effort to cleanup pm_runtime*get* calls, I detected a number
> of issues at the media subsystem.
> 
> Most of the patches here were submitted previously at:
> 
> 	https://lore.kernel.org/linux-media/cover.1619621413.git.mchehab+huawei@kernel.org/
> 
> This series contain just the bug fixes and other related issues that are
> present with the current code on media.

Series merged on my stage tree, at:
	https://git.linuxtv.org/media_stage.git/log/

I'll be merging it at media_tree (either for 5.13 or 5.14) after the
end of the merge window (likely next week).

Please let me know if you find any problems on it.

PS.: please notice that my stage tree can be rebased.

Regards,
Mauro

> 
> It address the points from the existing reviews. I also did my own
> set of reviews, in order to avoid regressions.
> 
> Changes from v4 of the previous changeset:
> 
> - reworked i2c/css RPM get logic;
> - dropped two patches that could cause regressions;
> - am437x: keep using pm_runtime_get_sync on suspend/resume;
> - atmel: fix the returned code and add a print on failures at start streaming;
> - simplify some checks for return code > 0;
> - mdk-vcodec: properly handle RPM errors at device on logic;
> - venus: rework venus_sys_error_handler() logic;
> - sti/delta: fix an issue at the error checking logic.
> 
> Mauro Carvalho Chehab (25):
>   staging: media: rkvdec: fix pm_runtime_get_sync() usage count
>   staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
>   media: venus: Rework error fail recover logic
>   media: s5p_cec: decrement usage count if disabled
>   media: i2c: ccs-core: return the right error code at suspend
>   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: hantro: do a PM resume earlier
>   media: marvel-ccic: fix some issues when getting pm_runtime
>   media: mdk-mdp: fix pm_runtime_get_sync() usage count
>   media: rcar_fdp1: simplify error check logic at fdp_open()
>   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 logic
>   media: s5p-jpeg: fix pm_runtime_get_sync() usage count
>   media: sti/delta: use pm_runtime_resume_and_get()
>   media: sunxi: fix pm_runtime_get_sync() usage count
>   media: sti/bdisp: fix pm_runtime_get_sync() usage count
>   media: exynos4-is: fix pm_runtime_get_sync() usage count
>   media: exynos-gsc: fix pm_runtime_get_sync() usage count
>   media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> 
>  drivers/media/cec/platform/s5p/s5p_cec.c      |  7 ++-
>  drivers/media/i2c/ccs/ccs-core.c              | 41 ++++++++-----
>  drivers/media/i2c/imx334.c                    |  7 ++-
>  drivers/media/platform/am437x/am437x-vpfe.c   | 15 ++++-
>  drivers/media/platform/atmel/atmel-isc-base.c | 30 +++++++---
>  drivers/media/platform/atmel/atmel-isi.c      | 19 ++++--
>  drivers/media/platform/exynos-gsc/gsc-core.c  | 11 ++--
>  drivers/media/platform/exynos-gsc/gsc-m2m.c   |  4 +-
>  .../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  |  5 +-
>  drivers/media/platform/exynos4-is/media-dev.c |  9 +--
>  drivers/media/platform/exynos4-is/mipi-csis.c | 10 ++--
>  .../media/platform/marvell-ccic/mcam-core.c   |  9 ++-
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  6 +-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  |  4 +-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   |  8 ++-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h   |  2 +-
>  drivers/media/platform/qcom/venus/core.c      | 59 +++++++++++++++----
>  drivers/media/platform/rcar_fdp1.c            | 28 ++++++---
>  drivers/media/platform/renesas-ceu.c          |  4 +-
>  drivers/media/platform/s5p-jpeg/jpeg-core.c   |  5 +-
>  drivers/media/platform/sh_vou.c               |  6 +-
>  drivers/media/platform/sti/bdisp/bdisp-v4l2.c |  7 ++-
>  drivers/media/platform/sti/delta/delta-v4l2.c |  8 +--
>  .../sunxi/sun8i-rotate/sun8i_rotate.c         |  2 +-
>  drivers/staging/media/hantro/hantro_drv.c     |  7 ++-
>  drivers/staging/media/imx/imx7-mipi-csis.c    |  7 +--
>  drivers/staging/media/rkvdec/rkvdec.c         |  2 +-
>  32 files changed, 220 insertions(+), 127 deletions(-)
> 



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 16/25] media: am437x: fix pm_runtime_get_sync() usage count
  2021-05-05  9:42 ` [PATCH 16/25] media: am437x: " Mauro Carvalho Chehab
  2021-05-05 12:32   ` Jonathan Cameron
@ 2021-05-08 13:01   ` Lad, Prabhakar
  1 sibling, 0 replies; 73+ messages in thread
From: Lad, Prabhakar @ 2021-05-08 13:01 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, LKML, linux-media

Hi Mauro,

Thank you for the patch.

On Wed, May 5, 2021 at 10:42 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> 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, avoiding
> a potential PM usage counter leak.
>
> While here, ensure that the driver will check if PM runtime
> resumed at vpfe_initialize_device().
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
Acked-by: Lad Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
Prabhakar


> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 6cdc77dda0e4..1c9cb9e05fdf 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe)
>         if (ret)
>                 return ret;
>
> -       pm_runtime_get_sync(vpfe->pdev);
> +       ret = pm_runtime_resume_and_get(vpfe->pdev);
> +       if (ret < 0)
> +               return ret;
>
>         vpfe_config_enable(&vpfe->ccdc, 1);
>
> @@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev)
>         pm_runtime_enable(&pdev->dev);
>
>         /* for now just enable it here instead of waiting for the open */
> -       pm_runtime_get_sync(&pdev->dev);
> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> +       if (ret < 0) {
> +               vpfe_err(vpfe, "Unable to resume device.\n");
> +               goto probe_out_v4l2_unregister;
> +       }
>
>         vpfe_ccdc_config_defaults(ccdc);
>
> @@ -2530,6 +2536,11 @@ static int vpfe_suspend(struct device *dev)
>
>         /* only do full suspend if streaming has started */
>         if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
> +               /*
> +                * ignore RPM resume errors here, as it is already too late.
> +                * A check like that should happen earlier, either at
> +                * open() or just before start streaming.
> +                */
>                 pm_runtime_get_sync(dev);
>                 vpfe_config_enable(ccdc, 1);
>
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-05-05 12:08   ` Jonathan Cameron
@ 2021-06-10  9:04     ` Eugen.Hristev
  2021-06-10  9:38       ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 73+ messages in thread
From: Eugen.Hristev @ 2021-06-10  9:04 UTC (permalink / raw)
  To: Jonathan.Cameron, mchehab+huawei
  Cc: linuxarm, mauro.chehab, alexandre.belloni, Ludovic.Desroches,
	mchehab, Nicolas.Ferre, linux-arm-kernel, linux-kernel,
	linux-media, Claudiu.Beznea

On 5/5/21 3:08 PM, Jonathan Cameron wrote:
> On Wed, 5 May 2021 11:41:58 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
>> There are several issues in the way the atmel driver handles
>> pm_runtime_get_sync():
>>
>> - it doesn't check return codes;
>> - it doesn't properly decrement the usage_count on all places;
>> - it starts streaming even if pm_runtime_get_sync() fails.
>> - while it tries to get pm_runtime at the clock enable logic,
>>    it doesn't check if the operation was suceeded.
>>
>> Replace all occurrences of it to use the new kAPI:
>> pm_runtime_resume_and_get(), which ensures that, if the
>> return code is not negative, the usage_count was incremented.
>>
>> With that, add additional checks when this is called, in order
>> to ensure that errors will be properly addressed.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> Hi Mauro, I don't know media enough to know what is the right answer
> but in some of this series, a failure in
> pm_runtime_resume_and_get() leads to a bunch of buffer cleanup
> (patch 22 being an example) and in others return happens without doing
> that cleanup.
> 
> It might be both are safe, or I'm missing something else, but I'm
> certainly not confident enough to give any tags on this one as a result
> of that mismatch.
> 
>> ---
>>   drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
>>   drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
>>   2 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>> index fe3ec8d0eaee..ce8e1351fa53 100644
>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>   static int isc_clk_prepare(struct clk_hw *hw)
>>   {
>>        struct isc_clk *isc_clk = to_isc_clk(hw);
>> +     int ret;
>>
>> -     if (isc_clk->id == ISC_ISPCK)
>> -             pm_runtime_get_sync(isc_clk->dev);
>> +     if (isc_clk->id == ISC_ISPCK) {
>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }

Hi Mauro,

With this patch, the ISC is broken on latest media tree. It looks like 
pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and 
thus, the probe of the driver fails:

atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
atmel-sama5d2-isc: probe of f0008000.isc failed with error -13


Could you point out how I could fix this ? Maybe the isc_clk->dev is not 
properly handled/initialized in some other part of the code ?

Thanks !
Eugen


>>
>>        return isc_wait_clk_stable(hw);
>>   }
>> @@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
>>   {
>>        struct isc_clk *isc_clk = to_isc_clk(hw);
>>        u32 status;
>> +     int ret;
>>
>> -     if (isc_clk->id == ISC_ISPCK)
>> -             pm_runtime_get_sync(isc_clk->dev);
>> +     if (isc_clk->id == ISC_ISPCK) {
>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>> +             if (ret < 0)
>> +                     return 0;
>> +     }
>>
>>        regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
>>
>> @@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>                goto err_start_stream;
>>        }
>>
>> -     pm_runtime_get_sync(isc->dev);
>> +     ret = pm_runtime_resume_and_get(isc->dev);
>> +     if (ret < 0) {
>> +             v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
>> +                      ret);
>> +             goto err_pm_get;
>> +     }
>>
>>        ret = isc_configure(isc);
>>        if (unlikely(ret))
>> @@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>
>>   err_configure:
>>        pm_runtime_put_sync(isc->dev);
>> -
>> +err_pm_get:
>>        v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>>
>>   err_start_stream:
>> @@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
>>        u32 baysel;
>>        unsigned long flags;
>>        u32 min, max;
>> +     int ret;
>>
>>        /* streaming is not active anymore */
>>        if (isc->stop)
>> @@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
>>        ctrls->hist_id = hist_id;
>>        baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
>>
>> -     pm_runtime_get_sync(isc->dev);
>> +     ret = pm_runtime_resume_and_get(isc->dev);
>> +     if (ret < 0)
>> +             return;
>>
>>        /*
>>         * only update if we have all the required histograms and controls
>> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
>> index e392b3efe363..5b1dd358f2e6 100644
>> --- a/drivers/media/platform/atmel/atmel-isi.c
>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>> @@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>        struct frame_buffer *buf, *node;
>>        int ret;
>>
>> -     pm_runtime_get_sync(isi->dev);
>> +     ret = pm_runtime_resume_and_get(isi->dev);
>> +     if (ret < 0)
>> +             return ret;
> This is the case I'm referring to above.
> 
>>
>>        /* Enable stream on the sub device */
>>        ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>> @@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
>>        return 0;
>>   }
>>
>> -static void isi_camera_set_bus_param(struct atmel_isi *isi)
>> +static int isi_camera_set_bus_param(struct atmel_isi *isi)
>>   {
>>        u32 cfg1 = 0;
>> +     int ret;
>>
>>        /* set bus param for ISI */
>>        if (isi->pdata.hsync_act_low)
>> @@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
>>        cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>>
>>        /* Enable PM and peripheral clock before operate isi registers */
>> -     pm_runtime_get_sync(isi->dev);
>> +     ret = pm_runtime_resume_and_get(isi->dev);
>> +     if (ret < 0)
>> +             return ret;
>>
>>        isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>        isi_writel(isi, ISI_CFG1, cfg1);
>>
>>        pm_runtime_put(isi->dev);
>> +
>> +     return 0;
>>   }
>>
>>   /* -----------------------------------------------------------------------*/
>> @@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>                dev_err(isi->dev, "No supported mediabus format found\n");
>>                return ret;
>>        }
>> -     isi_camera_set_bus_param(isi);
>> +     ret = isi_camera_set_bus_param(isi);
>> +     if (ret) {
>> +             dev_err(isi->dev, "Can't wake up device\n");
>> +             return ret;
>> +     }
>>
>>        ret = isi_set_default_fmt(isi);
>>        if (ret) {
> 


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-06-10  9:04     ` Eugen.Hristev
@ 2021-06-10  9:38       ` Mauro Carvalho Chehab
  2021-06-10 12:00         ` Eugen.Hristev
  0 siblings, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-06-10  9:38 UTC (permalink / raw)
  To: Eugen.Hristev
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

Em Thu, 10 Jun 2021 09:04:07 +0000
<Eugen.Hristev@microchip.com> escreveu:

> >> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >> index fe3ec8d0eaee..ce8e1351fa53 100644
> >> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>   static int isc_clk_prepare(struct clk_hw *hw)
> >>   {
> >>        struct isc_clk *isc_clk = to_isc_clk(hw);
> >> +     int ret;
> >>
> >> -     if (isc_clk->id == ISC_ISPCK)
> >> -             pm_runtime_get_sync(isc_clk->dev);
> >> +     if (isc_clk->id == ISC_ISPCK) {
> >> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +     }  
> 
> Hi Mauro,
> 
> With this patch, the ISC is broken on latest media tree. It looks like 
> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and 
> thus, the probe of the driver fails:
> 
> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> 
> 
> Could you point out how I could fix this ? Maybe the isc_clk->dev is not 
> properly handled/initialized in some other part of the code ?

Looking at RPM implementation:

	static int rpm_resume(struct device *dev, int rpmflags)
	{
...
        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth == 1 && dev->power.is_suspended
            && dev->power.runtime_status == RPM_ACTIVE)
                retval = 1;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;
...

It sounds that RPM is disabled for this clock. Did you call
pm_runtime_enable() before calling isc_clk_prepare()?

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-06-10  9:38       ` Mauro Carvalho Chehab
@ 2021-06-10 12:00         ` Eugen.Hristev
  2021-06-16  8:03           ` Mauro Carvalho Chehab
  2021-09-06  8:03           ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 73+ messages in thread
From: Eugen.Hristev @ 2021-06-10 12:00 UTC (permalink / raw)
  To: mchehab+huawei
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Em Thu, 10 Jun 2021 09:04:07 +0000
> <Eugen.Hristev@microchip.com> escreveu:
> 
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>    static int isc_clk_prepare(struct clk_hw *hw)
>>>>    {
>>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
>>>> +     int ret;
>>>>
>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>> +             if (ret < 0)
>>>> +                     return ret;
>>>> +     }
>>
>> Hi Mauro,
>>
>> With this patch, the ISC is broken on latest media tree. It looks like
>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>> thus, the probe of the driver fails:
>>
>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
>>
>>
>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>> properly handled/initialized in some other part of the code ?
> 
> Looking at RPM implementation:
> 
>          static int rpm_resume(struct device *dev, int rpmflags)
>          {
> ...
>          if (dev->power.runtime_error)
>                  retval = -EINVAL;
>          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>              && dev->power.runtime_status == RPM_ACTIVE)
>                  retval = 1;
>          else if (dev->power.disable_depth > 0)
>                  retval = -EACCES;
> ...
> 
> It sounds that RPM is disabled for this clock. Did you call
> pm_runtime_enable() before calling isc_clk_prepare()?
> 
> Thanks,
> Mauro
> 

I tried to call pm_runtime_enable for the clk at clk_init time, but 
doing that makes the runtime for the ISC fail like this:

atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-06-10 12:00         ` Eugen.Hristev
@ 2021-06-16  8:03           ` Mauro Carvalho Chehab
  2021-09-06  8:03           ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-06-16  8:03 UTC (permalink / raw)
  To: Eugen.Hristev
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

Em Thu, 10 Jun 2021 12:00:42 +0000
<Eugen.Hristev@microchip.com> escreveu:

> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Em Thu, 10 Jun 2021 09:04:07 +0000
> > <Eugen.Hristev@microchip.com> escreveu:
> >   
> >>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> index fe3ec8d0eaee..ce8e1351fa53 100644
> >>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>>>    static int isc_clk_prepare(struct clk_hw *hw)
> >>>>    {
> >>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
> >>>> +     int ret;
> >>>>
> >>>> -     if (isc_clk->id == ISC_ISPCK)
> >>>> -             pm_runtime_get_sync(isc_clk->dev);
> >>>> +     if (isc_clk->id == ISC_ISPCK) {
> >>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >>>> +             if (ret < 0)
> >>>> +                     return ret;
> >>>> +     }  
> >>
> >> Hi Mauro,
> >>
> >> With this patch, the ISC is broken on latest media tree. It looks like
> >> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
> >> thus, the probe of the driver fails:
> >>
> >> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> >> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> >>
> >>
> >> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
> >> properly handled/initialized in some other part of the code ?  
> > 
> > Looking at RPM implementation:
> > 
> >          static int rpm_resume(struct device *dev, int rpmflags)
> >          {
> > ...
> >          if (dev->power.runtime_error)
> >                  retval = -EINVAL;
> >          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> >              && dev->power.runtime_status == RPM_ACTIVE)
> >                  retval = 1;
> >          else if (dev->power.disable_depth > 0)
> >                  retval = -EACCES;
> > ...
> > 
> > It sounds that RPM is disabled for this clock. Did you call
> > pm_runtime_enable() before calling isc_clk_prepare()?
> > 
> > Thanks,
> > Mauro
> >   
> 
> I tried to call pm_runtime_enable for the clk at clk_init time, but 
> doing that makes the runtime for the ISC fail like this:
> 
> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!

Making RPM balanced is complex ;-)

Yet, clearly there's something weird at the PM code. I mean,
ignoring a -ENOACCESS error like the original code sounds wrong,
as it basically means that pm_runtime_get_sync() were doing
nothing (except by incrementing a refcount).

Some drivers call clk_prepare()/clk_unprepare() at their
.prepare/.unprepare ops. Those functions internally call
RPM get logic recursively, to ensure that the RPM
state of the parents are also at the right state.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-06-10 12:00         ` Eugen.Hristev
  2021-06-16  8:03           ` Mauro Carvalho Chehab
@ 2021-09-06  8:03           ` Mauro Carvalho Chehab
  2021-09-06  8:13             ` Eugen.Hristev
  1 sibling, 1 reply; 73+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-06  8:03 UTC (permalink / raw)
  To: Eugen.Hristev
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

Hi Eugen,

Em Thu, 10 Jun 2021 12:00:42 +0000
<Eugen.Hristev@microchip.com> escreveu:

> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Em Thu, 10 Jun 2021 09:04:07 +0000
> > <Eugen.Hristev@microchip.com> escreveu:
> >   
> >>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> index fe3ec8d0eaee..ce8e1351fa53 100644
> >>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>>>    static int isc_clk_prepare(struct clk_hw *hw)
> >>>>    {
> >>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
> >>>> +     int ret;
> >>>>
> >>>> -     if (isc_clk->id == ISC_ISPCK)
> >>>> -             pm_runtime_get_sync(isc_clk->dev);
> >>>> +     if (isc_clk->id == ISC_ISPCK) {
> >>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >>>> +             if (ret < 0)
> >>>> +                     return ret;
> >>>> +     }  
> >>
> >> Hi Mauro,
> >>
> >> With this patch, the ISC is broken on latest media tree. It looks like
> >> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
> >> thus, the probe of the driver fails:
> >>
> >> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> >> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13

What's the current status of this issue? 

If the bug still happens, we need a fix for it.

We might revert this patch, but this would just be masking some other
hidden bug.

Regards,
Mauro



> >>
> >>
> >> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
> >> properly handled/initialized in some other part of the code ?  
> > 
> > Looking at RPM implementation:
> > 
> >          static int rpm_resume(struct device *dev, int rpmflags)
> >          {
> > ...
> >          if (dev->power.runtime_error)
> >                  retval = -EINVAL;
> >          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> >              && dev->power.runtime_status == RPM_ACTIVE)
> >                  retval = 1;
> >          else if (dev->power.disable_depth > 0)
> >                  retval = -EACCES;
> > ...
> > 
> > It sounds that RPM is disabled for this clock. Did you call
> > pm_runtime_enable() before calling isc_clk_prepare()?
> > 
> > Thanks,
> > Mauro
> >   
> 
> I tried to call pm_runtime_enable for the clk at clk_init time, but 
> doing that makes the runtime for the ISC fail like this:
> 
> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-09-06  8:03           ` Mauro Carvalho Chehab
@ 2021-09-06  8:13             ` Eugen.Hristev
  2021-09-13 10:26               ` Eugen.Hristev
  0 siblings, 1 reply; 73+ messages in thread
From: Eugen.Hristev @ 2021-09-06  8:13 UTC (permalink / raw)
  To: mchehab+huawei
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

On 9/6/21 11:03 AM, Mauro Carvalho Chehab wrote:
> Hi Eugen,
> 
> Em Thu, 10 Jun 2021 12:00:42 +0000
> <Eugen.Hristev@microchip.com> escreveu:
> 
>> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Em Thu, 10 Jun 2021 09:04:07 +0000
>>> <Eugen.Hristev@microchip.com> escreveu:
>>>
>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>>>     static int isc_clk_prepare(struct clk_hw *hw)
>>>>>>     {
>>>>>>          struct isc_clk *isc_clk = to_isc_clk(hw);
>>>>>> +     int ret;
>>>>>>
>>>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>>>> +             if (ret < 0)
>>>>>> +                     return ret;
>>>>>> +     }
>>>>
>>>> Hi Mauro,
>>>>
>>>> With this patch, the ISC is broken on latest media tree. It looks like
>>>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>>>> thus, the probe of the driver fails:
>>>>
>>>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>>>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> 
> What's the current status of this issue?

Hi Mauro,

Currently, as far as I know, the ISC is broken in 5.14 and current Linux 
master. I plan to investigate this issue this week (or the next...), 
together with some other things. I want to tryout the PM part of the 
driver to see where is the problem.
When I come up with a fix or patch, I will send it on the mailing list.
If you have any ideas that I can try meanwhile, feel free to contact me 
to send a patch that I can test.


Eugen


> 
> If the bug still happens, we need a fix for it.
> 
> We might revert this patch, but this would just be masking some other
> hidden bug.
> 
> Regards,
> Mauro
> 
> 
> 
>>>>
>>>>
>>>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>>>> properly handled/initialized in some other part of the code ?
>>>
>>> Looking at RPM implementation:
>>>
>>>           static int rpm_resume(struct device *dev, int rpmflags)
>>>           {
>>> ...
>>>           if (dev->power.runtime_error)
>>>                   retval = -EINVAL;
>>>           else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>>>               && dev->power.runtime_status == RPM_ACTIVE)
>>>                   retval = 1;
>>>           else if (dev->power.disable_depth > 0)
>>>                   retval = -EACCES;
>>> ...
>>>
>>> It sounds that RPM is disabled for this clock. Did you call
>>> pm_runtime_enable() before calling isc_clk_prepare()?
>>>
>>> Thanks,
>>> Mauro
>>>
>>
>> I tried to call pm_runtime_enable for the clk at clk_init time, but
>> doing that makes the runtime for the ISC fail like this:
>>
>> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!
> 
> 
> 
> Thanks,
> Mauro
> 


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 08/25] media: atmel: properly get pm_runtime
  2021-09-06  8:13             ` Eugen.Hristev
@ 2021-09-13 10:26               ` Eugen.Hristev
  0 siblings, 0 replies; 73+ messages in thread
From: Eugen.Hristev @ 2021-09-13 10:26 UTC (permalink / raw)
  To: mchehab+huawei
  Cc: Jonathan.Cameron, linuxarm, mauro.chehab, alexandre.belloni,
	Ludovic.Desroches, mchehab, Nicolas.Ferre, linux-arm-kernel,
	linux-kernel, linux-media, Claudiu.Beznea

On 9/6/21 11:13 AM, Eugen Hristev - M18282 wrote:
> On 9/6/21 11:03 AM, Mauro Carvalho Chehab wrote:
>> Hi Eugen,
>>
>> Em Thu, 10 Jun 2021 12:00:42 +0000
>> <Eugen.Hristev@microchip.com> escreveu:
>>
>>> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Em Thu, 10 Jun 2021 09:04:07 +0000
>>>> <Eugen.Hristev@microchip.com> escreveu:
>>>>
>>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>>>>      static int isc_clk_prepare(struct clk_hw *hw)
>>>>>>>      {
>>>>>>>           struct isc_clk *isc_clk = to_isc_clk(hw);
>>>>>>> +     int ret;
>>>>>>>
>>>>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>>>>> +             if (ret < 0)
>>>>>>> +                     return ret;
>>>>>>> +     }
>>>>>
>>>>> Hi Mauro,
>>>>>
>>>>> With this patch, the ISC is broken on latest media tree. It looks like
>>>>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>>>>> thus, the probe of the driver fails:
>>>>>
>>>>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>>>>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
>>
>> What's the current status of this issue?
> 
> Hi Mauro,
> 
> Currently, as far as I know, the ISC is broken in 5.14 and current Linux
> master. I plan to investigate this issue this week (or the next...),
> together with some other things. I want to tryout the PM part of the
> driver to see where is the problem.
> When I come up with a fix or patch, I will send it on the mailing list.
> If you have any ideas that I can try meanwhile, feel free to contact me
> to send a patch that I can test.
> 
> 

Hi Mauro,

Regarding this issue, I sent a patch on the ML that should solve it.

https://patchwork.linuxtv.org/project/linux-media/patch/20210913102254.108638-1-eugen.hristev@microchip.com/

Eugen

> Eugen
> 
> 
>>
>> If the bug still happens, we need a fix for it.
>>
>> We might revert this patch, but this would just be masking some other
>> hidden bug.
>>
>> Regards,
>> Mauro
>>
>>
>>
>>>>>
>>>>>
>>>>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>>>>> properly handled/initialized in some other part of the code ?
>>>>
>>>> Looking at RPM implementation:
>>>>
>>>>            static int rpm_resume(struct device *dev, int rpmflags)
>>>>            {
>>>> ...
>>>>            if (dev->power.runtime_error)
>>>>                    retval = -EINVAL;
>>>>            else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>>>>                && dev->power.runtime_status == RPM_ACTIVE)
>>>>                    retval = 1;
>>>>            else if (dev->power.disable_depth > 0)
>>>>                    retval = -EACCES;
>>>> ...
>>>>
>>>> It sounds that RPM is disabled for this clock. Did you call
>>>> pm_runtime_enable() before calling isc_clk_prepare()?
>>>>
>>>> Thanks,
>>>> Mauro
>>>>
>>>
>>> I tried to call pm_runtime_enable for the clk at clk_init time, but
>>> doing that makes the runtime for the ISC fail like this:
>>>
>>> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!
>>
>>
>>
>> Thanks,
>> Mauro
>>
> 


^ permalink raw reply	[flat|nested] 73+ messages in thread

end of thread, other threads:[~2021-09-13 10:26 UTC | newest]

Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05  9:41 [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab
2021-05-05  9:41 ` [PATCH 01/25] staging: media: rkvdec: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05 12:23   ` Jonathan Cameron
2021-05-05  9:41 ` [PATCH 02/25] staging: media: imx7-mipi-csis: " Mauro Carvalho Chehab
2021-05-05 11:06   ` Jonathan Cameron
2021-05-05 11:17     ` Mauro Carvalho Chehab
2021-05-05 13:56     ` Rui Miguel Silva
2021-05-05 14:23       ` Mauro Carvalho Chehab
2021-05-05  9:41 ` [PATCH 03/25] media: venus: Rework error fail recover logic Mauro Carvalho Chehab
2021-05-05 11:05   ` Jonathan Cameron
2021-05-05  9:41 ` [PATCH 04/25] media: s5p_cec: decrement usage count if disabled Mauro Carvalho Chehab
2021-05-05 12:24   ` Jonathan Cameron
2021-05-05  9:41 ` [PATCH 05/25] media: i2c: ccs-core: return the right error code at suspend Mauro Carvalho Chehab
2021-05-05 12:24   ` Jonathan Cameron
2021-05-05 12:51   ` Sakari Ailus
2021-05-05  9:41 ` [PATCH 06/25] media: i2c: imx334: fix the pm runtime get logic Mauro Carvalho Chehab
2021-05-05 11:10   ` Jonathan Cameron
2021-05-05 11:24     ` Mauro Carvalho Chehab
2021-05-05 12:26       ` Jonathan Cameron
2021-05-05  9:41 ` [PATCH 07/25] media: exynos-gsc: don't resume at remove time Mauro Carvalho Chehab
2021-05-05 12:27   ` Jonathan Cameron
2021-05-05  9:41 ` [PATCH 08/25] media: atmel: properly get pm_runtime Mauro Carvalho Chehab
2021-05-05 12:08   ` Jonathan Cameron
2021-06-10  9:04     ` Eugen.Hristev
2021-06-10  9:38       ` Mauro Carvalho Chehab
2021-06-10 12:00         ` Eugen.Hristev
2021-06-16  8:03           ` Mauro Carvalho Chehab
2021-09-06  8:03           ` Mauro Carvalho Chehab
2021-09-06  8:13             ` Eugen.Hristev
2021-09-13 10:26               ` Eugen.Hristev
2021-05-05  9:41 ` [PATCH 09/25] media: hantro: do a PM resume earlier Mauro Carvalho Chehab
2021-05-05 11:34   ` Jonathan Cameron
2021-05-05 13:22   ` Ezequiel Garcia
2021-05-05 13:46     ` Mauro Carvalho Chehab
2021-05-05 14:01       ` Ezequiel Garcia
2021-05-05 14:15         ` Mauro Carvalho Chehab
2021-05-05  9:42 ` [PATCH 10/25] media: marvel-ccic: fix some issues when getting pm_runtime Mauro Carvalho Chehab
2021-05-05  9:42 ` [PATCH 11/25] media: mdk-mdp: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05  9:42 ` [PATCH 12/25] media: rcar_fdp1: simplify error check logic at fdp_open() Mauro Carvalho Chehab
2021-05-05  9:48   ` Sergei Shtylyov
2021-05-05  9:42 ` [PATCH 13/25] media: rcar_fdp1: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05 12:31   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 14/25] media: renesas-ceu: Properly check for PM errors Mauro Carvalho Chehab
2021-05-05  9:56   ` Jacopo Mondi
2021-05-05  9:42 ` [PATCH 15/25] media: s5p: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05 12:31   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 16/25] media: am437x: " Mauro Carvalho Chehab
2021-05-05 12:32   ` Jonathan Cameron
2021-05-08 13:01   ` Lad, Prabhakar
2021-05-05  9:42 ` [PATCH 17/25] media: sh_vou: " Mauro Carvalho Chehab
2021-05-05  9:42 ` [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic Mauro Carvalho Chehab
2021-05-05 12:32   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05 12:33   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 20/25] media: sti/delta: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
2021-05-05 12:01   ` Jonathan Cameron
2021-05-05 12:33     ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 21/25] media: sunxi: fix pm_runtime_get_sync() usage count Mauro Carvalho Chehab
2021-05-05  9:42 ` [PATCH 22/25] media: sti/bdisp: " Mauro Carvalho Chehab
2021-05-05 12:34   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 23/25] media: exynos4-is: " Mauro Carvalho Chehab
2021-05-05 12:20   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 24/25] media: exynos-gsc: " Mauro Carvalho Chehab
2021-05-05 12:34   ` Jonathan Cameron
2021-05-05  9:42 ` [PATCH 25/25] media: i2c: ccs-core: " Mauro Carvalho Chehab
2021-05-05 10:34   ` Sakari Ailus
2021-05-05 10:57     ` Mauro Carvalho Chehab
2021-05-05 10:58       ` Mauro Carvalho Chehab
2021-05-05 12:35         ` Jonathan Cameron
2021-05-05 14:06           ` Mauro Carvalho Chehab
2021-05-05 16:36             ` Jonathan Cameron
2021-05-05 11:02       ` Sakari Ailus
2021-05-06 15:11 ` [PATCH 00/25] Fix some PM runtime issues at the media subsystem Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).